build: fix release.yml build errors - #452
Conversation
❌ This PR targets
|
github-actions
Bot
added
ci
example-app
WalkthroughThe PR updates native build configuration to use ChangesBuild system, Python binding, CI, and dev tooling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
❌ This PR targets
|
coderabbitai
Bot
added
c/c++
bd9cb8c to
4b79b62
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 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 @.github/workflows/pr-target-check.yml:
- Around line 19-24: The permissions section in the workflow job is missing the
required `issues: write` permission for the peter-evans/create-or-update-comment
action. Add `issues: write` permission alongside the existing `pull-requests:
write` permission in the permissions block so that the create-or-update-comment
step has the necessary API scopes to execute successfully.
In @.github/workflows/release.yml:
- Around line 5-7: The release workflow lacks concurrency configuration at the
workflow level, which allows multiple release pipelines to run simultaneously
and potentially race on tags and artifacts. Add a concurrency block to the
workflow after the trigger configuration (the on section with branches: [main,
dev] for push and pull_request) with a group identifier based on the git
reference (github.ref) and set cancel-in-progress to true so that any
in-progress release runs are cancelled when a new one starts on the same branch.
- Around line 154-157: The actions/checkout action in the release workflow is
missing the persist-credentials setting, which leaves Git credentials exposed
during downstream build and publish steps. Add persist-credentials: false to the
with section of the actions/checkout@900f2210b1d28bbbd0bd22d17926b9e224e8f231
action to limit credential exposure and follow least-privilege secret handling
practices.
- Around line 135-142: The softprops/action-gh-release step uses the if
condition that checks if the component was created or if it's a dry run, but
this doesn't align with the conditions that gate the artifact packaging steps
that set env.ARCHIVE. This causes the release step to potentially run without
artifacts being packaged. Identify the actual conditions under which the
artifact packaging steps execute (the steps that set env.ARCHIVE), and update
the if condition in the softprops/action-gh-release step to match those same
conditions. This ensures the release step only runs when artifacts have been
prepared and env.ARCHIVE is properly set.
- Around line 276-297: The pypa/gh-action-pypi-publish steps for both production
PyPI and TestPyPI publishing are contained within a job that only runs when
py_created == 'true', which prevents the dry-run path from being reachable.
Update the job-level conditional (the if condition on the job definition) to
also allow execution when a dry-run is being performed, so that the step-level
conditionals can properly route to either production PyPI or TestPyPI based on
the is_dry_run flag.
In `@bindings/c/include/cimg2num.h`:
- Around line 45-46: The min_thickness parameter has been added to the
img2num_ImageToSvgConfig struct and img2num_labels_to_svg function in the C API
but lacks test coverage. Add test cases in the bindings/c/ test directory that
verify the min_thickness parameter is correctly passed through the C binding to
the core implementation. Create tests that validate SVG output behavior with
various min_thickness values (including edge cases like zero, small, and large
values) to ensure regions are included or excluded from the SVG based on the
thickness threshold.
In `@CMakeLists.txt`:
- Around line 22-23: Change the default value of the
IMG2NUM_DEBUG_CACHE_VARIABLES_DUMP option from ON to OFF to prevent sensitive
information (such as cache variables and inputs) from being logged and
potentially leaked in CI logs. Additionally, review lines 56-60 for similar
cache variable dumping options that may have the same issue and apply the same
fix by changing their defaults from ON to OFF.
- Around line 72-82: The conditional logic in the CMakeLists.txt file uses
elseif statements that make the binding selection mutually exclusive, preventing
valid combinations like building both C and JavaScript bindings for WASM.
Restructure this block to allow independent conditions for each binding type
instead of using elseif chains. Keep IMG2NUM_BUILD_PYTHON as mutually exclusive
if needed, but convert the IMG2NUM_BUILD_C and EMSCRIPTEN conditions to separate
if statements (or restructure appropriately) so that when both IMG2NUM_BUILD_C
and EMSCRIPTEN are true, both the C bindings and JS bindings can be added
together.
In `@core/include/img2num.h`:
- Around line 83-86: The function declaration for labels_to_svg in the header
file is missing a default argument value for the min_thickness parameter, while
the implementation in labels_to_svg.cpp includes a default value of 0. This
mismatch breaks source compatibility by requiring callers to provide all
arguments. Add the default value = 0 to the min_thickness parameter in the
labels_to_svg function declaration in the header file to match the
implementation and restore API compatibility.
In `@core/include/internal/contours.h`:
- Around line 64-67: The coupled_smooth_junctions function passes the junctions
parameter by value, causing unnecessary copying of the image-sized mask buffer
on every invocation. Since the junctions parameter is never mutated within the
function, change it to be passed as a const reference instead. Apply this
parameter modification to both the function declaration in contours.h and the
function definition in contours.cpp, then run ./img2num format-cpp to ensure the
changes comply with the clang-format style guide.
In `@core/include/internal/graph.h`:
- Around line 52-56: The getPixel function validates the coordinate bounds
against width and height parameters but does not verify that the image vector's
actual size matches the expected dimensions (w * h). Add a validation check at
the beginning of the getPixel function to ensure that img.size() equals w * h
before performing the index calculation, returning 0 or handling the error case
appropriately if the sizes are inconsistent. This prevents out-of-bounds access
when the caller provides mismatched dimensions.
In `@core/src/internal/bezier.cpp`:
- Around line 163-180: The code accesses fixed[i] without verifying that i is
within bounds of the fixed vector, which causes undefined behavior when chains
and fixed have mismatched sizes. Add a bounds check to ensure i < fixed.size()
before accessing fixed[i] in the loop where chains are iterated. This check
should be applied before or within the condition that currently accesses
fixed[i][k] to prevent out-of-bounds access.
In `@core/src/internal/contours.cpp`:
- Around line 682-691: In the updateLockedMasks function, before accessing
junctions[idx] where idx is computed from pt.y * width + pt.x, add bounds
validation to ensure the index is non-negative and less than the size of the
junctions vector. Insert a conditional check after computing idx that verifies
idx is within valid bounds before dereferencing junctions[idx] to prevent
out-of-range memory access and potential crashes on invalid point coordinates.
- Around line 835-840: The `coupled_smooth_junctions` function passes the
`junctions` parameter by value, which unnecessarily copies a `width*height`
buffer. Change the parameter type from `std::vector<uint8_t>` to `const
std::vector<uint8_t>&` to pass by const reference instead. Apply this change
consistently in both the function declaration and its definition to avoid the
expensive copy operation.
In `@core/src/internal/douglas_peucker.cpp`:
- Around line 104-117: In the dp_curve_reduction function, add a defensive
bounds check before accessing fixed[i] in the loop that iterates over chains.
Before accessing the fixed vector at index i, verify that i is within the valid
range of fixed.size(). If the index is out of bounds, either skip processing
that chain or use a default value for the fixed mask. This ensures the function
gracefully handles cases where the caller passes mismatched vector sizes rather
than causing undefined behavior from out-of-bounds access.
In `@core/src/internal/image_utils.cpp`:
- Around line 120-123: The validation check for num_thresholds only guards
against values less than or equal to zero, but does not validate the upper
bound. When num_thresholds exceeds 255, the integer division in the REGION_SIZE
calculation (255 / num_thresholds) evaluates to zero, creating undefined
behavior in the quantization logic. Add an additional validation check after the
existing num_thresholds <= 0 guard to ensure num_thresholds does not exceed 255,
either by clamping the value to a valid range or returning early if the upper
bound is violated, before the REGION_SIZE constant is computed.
In `@Dockerfile.dev`:
- Around line 146-149: The `just` package manager installer script is being
downloaded and executed without any checksum verification, which poses a
supply-chain security risk. Replace the current approach in the RUN instruction
that downloads the unverified installer script from
https://just.systems/install.sh with either: (1) using official pre-built
binaries for the target architecture with SHA256SUMS verification from the
official `just` repository, or (2) if continuing with the installer script,
download both the installer script and the corresponding SHA256SUMS file, verify
the script's checksum matches the official one before execution, and then
proceed with running the verified script. This will ensure reproducibility and
supply-chain integrity by validating the integrity of downloaded artifacts.
In `@example-apps/console-cpp/main.cpp`:
- Line 68: The variable res_svg is created by calling img2num::labels_to_svg but
is never used afterwards, which causes a build failure with strict compiler
settings. Either remove the res_svg variable declaration entirely if the
generated SVG is not needed, or use the variable by writing it to output, saving
it to a file, printing it, or passing it to another function that consumes it.
In `@Justfile`:
- Around line 52-65: The build and clean recipes have case statements that
silently succeed when an unrecognized target is provided, which can hide typos
and cause scripts to fail unexpectedly. Add a default case clause to both the
build target and clean target recipes using the pattern `*) echo "Error:
unsupported target" >&2; exit 1 ;;` at the end of each case statement to
explicitly fail when an invalid target is passed, ensuring unknown targets are
caught immediately.
In `@packages/py/img2num/api.py`:
- Around line 64-65: The `labels_to_svg` function currently has `min_thickness`
as a required parameter, which breaks backward compatibility for existing
callers. Add a default value of `0` to the `min_thickness` parameter in the
function signature to restore backward compatibility while continuing to forward
it to the downstream `_labels_to_svg` call.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ba07b7c-2df3-470f-8041-d3e7d24d451a
⛔ Files ignored due to path filters (5)
docs/static/img/readme-demo/output-aerial-view-mountains_pexels-pixabay-51373.svgis excluded by!**/*.svgdocs/static/img/readme-demo/output-margate-garden.svgis excluded by!**/*.svgdocs/static/img/readme-demo/output-ring-on-hand.svgis excluded by!**/*.svgtest.jpgis excluded by!**/*.jpguv.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
.coderabbit.yaml.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/ISSUE_TEMPLATE/good_first_issue.yml.github/ISSUE_TEMPLATE/refactor.yml.github/PULL_REQUEST_TEMPLATE.md.github/workflows/pr-check.yml.github/workflows/pr-target-check.yml.github/workflows/release.yml.github/workflows/stale.ymlCMakeLists.txtDockerfile.devJustfilebindings/c/CMakeLists.txtbindings/c/include/cimg2num.hbindings/c/src/cimg2num.cppbindings/js/src/wasm_wrapper.cbindings/py/CMakeLists.txtbindings/py/src/img2num_pybind.cppcore/CMakeLists.txtcore/include/img2num.hcore/include/internal/LABAPixel.hcore/include/internal/LABPixel.hcore/include/internal/RGBAPixel.hcore/include/internal/RGBPixel.hcore/include/internal/bezier.hcore/include/internal/contours.hcore/include/internal/douglas_peucker.hcore/include/internal/gpu.hcore/include/internal/graph.hcore/include/internal/shared_contours.hcore/src/internal/bezier.cppcore/src/internal/contours.cppcore/src/internal/douglas_peucker.cppcore/src/internal/graph.cppcore/src/internal/image_to_svg.cppcore/src/internal/image_utils.cppcore/src/internal/kmeans_gpu.cppcore/src/internal/labels_to_svg.cppcore/src/internal/shared_contours.cppexample-apps/console-c/CMakeLists.txtexample-apps/console-c/main.cexample-apps/console-cpp/CMakeLists.txtexample-apps/console-cpp/main.cppexample-apps/console-py/main.pypackages/js/safeWasmWrappers.jspackages/py/img2num/api.pypyproject.toml
💤 Files with no reviewable changes (4)
- .github/workflows/stale.yml
- .github/ISSUE_TEMPLATE/good_first_issue.yml
- .github/ISSUE_TEMPLATE/refactor.yml
- .github/workflows/pr-check.yml
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: build-c-cpp-native (macos-latest, c)
- GitHub Check: build-c-cpp-native (ubuntu-latest, c)
- GitHub Check: build-c-cpp-native (ubuntu-latest, cpp)
- GitHub Check: build-c-cpp-native (macos-latest, cpp)
- GitHub Check: build-py (ubuntu-latest)
- GitHub Check: build-py (windows-latest)
- GitHub Check: build-py (macos-latest)
- GitHub Check: Build C/C++ / Build Python
- GitHub Check: Build C/C++ / Build WASM (bindings/js)
- GitHub Check: Build C/C++ / Build C & C++
- GitHub Check: Build Documentation Site / Build Docusaurus Site
- GitHub Check: Lint & Validate Code
- GitHub Check: Analyze (c-cpp)
🧰 Additional context used
📓 Path-based instructions (14)
**/*.{hpp,cpp,c,h}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{hpp,cpp,c,h}: Use 4-space indentation for C/C++ files
Maintain 120 character maximum line length for C/C++ files
Files:
core/include/internal/bezier.hcore/include/internal/LABPixel.hcore/include/internal/douglas_peucker.hcore/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppcore/include/internal/shared_contours.hbindings/py/src/img2num_pybind.cppcore/include/internal/RGBPixel.hcore/include/internal/gpu.hcore/include/internal/LABAPixel.hcore/include/img2num.hcore/src/internal/kmeans_gpu.cppcore/include/internal/RGBAPixel.hcore/src/internal/labels_to_svg.cppcore/src/internal/image_utils.cppcore/include/internal/contours.hcore/src/internal/douglas_peucker.cppcore/include/internal/graph.hcore/src/internal/contours.cppcore/src/internal/bezier.cppbindings/c/src/cimg2num.cppexample-apps/console-c/main.cbindings/c/include/cimg2num.hcore/src/internal/shared_contours.cppbindings/js/src/wasm_wrapper.ccore/src/internal/graph.cpp
**/*.{cpp,cc,cxx,c++,h,hpp,hxx,h++}
📄 CodeRabbit inference engine (.clang-format)
**/*.{cpp,cc,cxx,c++,h,hpp,hxx,h++}: Use LLVM coding style as the base style for C++ code
Use C++20 standard for all C++ code
Use an indent width of 4 spaces for C++ code
Limit line length to 100 columns in C++ code
Use 4 spaces for tab width in C++ code
Use C++11 braced list style with space before braced lists in C++ code
Attach opening braces to the same line (BreakBeforeBraces: Attach) in C++ code
Never pack constructor initializer lists on a single line in C++ code
Break constructor initializer lists before comma in C++ code
Use 4 spaces for constructor initializer list indentation in C++ code
Use block indent alignment after opening brackets in C++ code
Align pointers to the left in C++ code
Regroup and organize include blocks in C++ code
Sort includes case-insensitively in C++ code
Do not indent extern "C" blocks in C++ code
Allow short lambdas only inline on a single line in C++ code
Do not allow short functions on a single line in C++ code
Files:
core/include/internal/bezier.hcore/include/internal/LABPixel.hcore/include/internal/douglas_peucker.hcore/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppcore/include/internal/shared_contours.hbindings/py/src/img2num_pybind.cppcore/include/internal/RGBPixel.hcore/include/internal/gpu.hcore/include/internal/LABAPixel.hcore/include/img2num.hcore/src/internal/kmeans_gpu.cppcore/include/internal/RGBAPixel.hcore/src/internal/labels_to_svg.cppcore/src/internal/image_utils.cppcore/include/internal/contours.hcore/src/internal/douglas_peucker.cppcore/include/internal/graph.hcore/src/internal/contours.cppcore/src/internal/bezier.cppbindings/c/src/cimg2num.cppbindings/c/include/cimg2num.hcore/src/internal/shared_contours.cppcore/src/internal/graph.cpp
core/**/*.{cpp,c,h,hpp}
⚙️ CodeRabbit configuration file
core/**/*.{cpp,c,h,hpp}: This is the Img2Num core C/C++ library. Review for:
- Memory safety: null pointer dereferences, use-after-free, buffer overflows.
- Correct RAII usage and smart pointer idioms.
- Adherence to the .clang-format style; formatting must be applied via
./img2num format-cpp(or./img2num format-wasmfor WASM modules),
NOT by calling clang-format directly.- When suggesting build/test steps, always use the Docker-first wrapper scripts
(./img2num,img2num.ps1, orimg2num.bat) rather than direct tool invocations,
since contributors may not have dependencies installed locally.- Must have Doxygen docstrings in the API (
img2num.h) files. This is important!
Files:
core/include/internal/bezier.hcore/include/internal/LABPixel.hcore/include/internal/douglas_peucker.hcore/src/internal/image_to_svg.cppcore/include/internal/shared_contours.hcore/include/internal/RGBPixel.hcore/include/internal/gpu.hcore/include/internal/LABAPixel.hcore/include/img2num.hcore/src/internal/kmeans_gpu.cppcore/include/internal/RGBAPixel.hcore/src/internal/labels_to_svg.cppcore/src/internal/image_utils.cppcore/include/internal/contours.hcore/src/internal/douglas_peucker.cppcore/include/internal/graph.hcore/src/internal/contours.cppcore/src/internal/bezier.cppcore/src/internal/shared_contours.cppcore/src/internal/graph.cpp
**/*.{cpp,c,h,hpp}
⚙️ CodeRabbit configuration file
**/*.{cpp,c,h,hpp}: - For any C/C++ code outside core/ (e.g. bindings, example-apps), apply the same
memory-safety and style standards as the core. Formatting via./img2num format-cpp.
- Doxygen docstrings are required.
Files:
core/include/internal/bezier.hcore/include/internal/LABPixel.hcore/include/internal/douglas_peucker.hcore/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppcore/include/internal/shared_contours.hbindings/py/src/img2num_pybind.cppcore/include/internal/RGBPixel.hcore/include/internal/gpu.hcore/include/internal/LABAPixel.hcore/include/img2num.hcore/src/internal/kmeans_gpu.cppcore/include/internal/RGBAPixel.hcore/src/internal/labels_to_svg.cppcore/src/internal/image_utils.cppcore/include/internal/contours.hcore/src/internal/douglas_peucker.cppcore/include/internal/graph.hcore/src/internal/contours.cppcore/src/internal/bezier.cppbindings/c/src/cimg2num.cppexample-apps/console-c/main.cbindings/c/include/cimg2num.hcore/src/internal/shared_contours.cppbindings/js/src/wasm_wrapper.ccore/src/internal/graph.cpp
**
⚙️ CodeRabbit configuration file
**: # Contributing to Img2NumWant to contribute to Img2Num? There are a few things you need to know.
We wrote a contribution guide to help you get started.
A few important points:
- Add tests with your PR — new features and bug fixes must include tests where appropriate. PRs without tests are unlikely to be approved.
- Follow the repository's coding style rules.
- Use the issue and PR templates when filing issues or submitting code. Your PR will be rejected if you don't.
If you're unsure what to change, open a discussion and someone will assist you.
Questions?
If you have questions or need help:
- Open a discussion
- Create an issue
- Check existing PRs for ideas
Thank you for improving Img2Num! 🎨🚀
**: BasedOnStyle: LLVM
Standard: c++20--- Basic formatting ---
IndentWidth: 4
ColumnLimit: 100
TabWidth: 4--- Braces ---
Cpp11BracedListStyle: true
SpaceBeforeCpp11BracedList: true
BreakBeforeBraces: Attach--- Braced initializers ---
Cpp11BracedListStyle: true
--- Constructor initializer lists ---
PackConstructorInitializers: Never
BreakConstructorInitializers: BeforeComma
ConstructorInitializerIndentWidth: 4--- Alignment ---
AlignAfterOpenBracket: BlockIndent
--- Pointers ---
PointerAlignment: Left
--- Includes ---
IncludeBlocks: Regroup
SortIncludes: CaseInsensitive--- Extern "C" cleanliness ---
IndentExternBlock: NoIndent
--- Lambdas ---
AllowShortLambdasOnASingleLine: Inline
--- Functions ---
AllowShortFunctionsOnASingleLine: None
**: root = true-------------------------
Global defaults
-------------------------
[*]
charset = utf-8
end_of_line = lf
indent_style = space
indent_size = 2
trim_trailing_whitespace = true
insert_final_newline = true
max_line_lengt...
Files:
core/include/internal/bezier.hcore/include/internal/LABPixel.hDockerfile.devcore/include/internal/douglas_peucker.hexample-apps/console-c/CMakeLists.txtcore/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppexample-apps/console-py/main.pycore/include/internal/shared_contours.hbindings/py/src/img2num_pybind.cppcore/include/internal/RGBPixel.hcore/include/internal/gpu.hcore/include/internal/LABAPixel.hpackages/py/img2num/api.pycore/CMakeLists.txtcore/include/img2num.hcore/src/internal/kmeans_gpu.cpppyproject.tomlcore/include/internal/RGBAPixel.hcore/src/internal/labels_to_svg.cppcore/src/internal/image_utils.cppexample-apps/console-cpp/CMakeLists.txtcore/include/internal/contours.hcore/src/internal/douglas_peucker.cppcore/include/internal/graph.hJustfilepackages/js/safeWasmWrappers.jscore/src/internal/contours.cppcore/src/internal/bezier.cppbindings/c/src/cimg2num.cppexample-apps/console-c/main.cbindings/c/include/cimg2num.hcore/src/internal/shared_contours.cppbindings/js/src/wasm_wrapper.cCMakeLists.txtbindings/c/CMakeLists.txtbindings/py/CMakeLists.txtcore/src/internal/graph.cpp
Dockerfile*
⚙️ CodeRabbit configuration file
Dockerfile*: Review with Hadolint rules. Prefer multi-stage builds, minimal base images,
and pinned image tags. Ensure no secrets are baked into layers.
Files:
Dockerfile.dev
**/*.txt
📄 CodeRabbit inference engine (.editorconfig)
**/*.txt: Do not enforce maximum line length for text files
Use 2-space indentation for text files
Files:
example-apps/console-c/CMakeLists.txtcore/CMakeLists.txtexample-apps/console-cpp/CMakeLists.txtCMakeLists.txtbindings/c/CMakeLists.txtbindings/py/CMakeLists.txt
example-apps/**
⚙️ CodeRabbit configuration file
example-apps/**: - Example applications. These are for demonstration; keep them minimal and ensure
they correctly reflect the public API. Flag any use of internal/private APIs.
- IMPORTANT: These example apps must be a good reflection of how to use Img2Num's
library, which means that they need good comments and must have clean code. This
is meant to be external and is designed for others to use to get started.
Files:
example-apps/console-c/CMakeLists.txtexample-apps/console-cpp/main.cppexample-apps/console-py/main.pyexample-apps/console-cpp/CMakeLists.txtexample-apps/console-c/main.c
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: GitHub Actions workflows. Review for:
- SHA-pinned action versions for third-party actions (security best practice).
- Secrets accessed only via ${{ secrets.* }} — never hardcoded.
- Least-privilege permissions on each job/workflow.
- Correct job dependency ordering (needs:) and if/condition logic.
Files:
.github/workflows/pr-target-check.yml.github/workflows/release.yml
.coderabbit.yaml
📄 CodeRabbit inference engine (Custom checks)
.coderabbit.yaml: Warn if a PR introduces new programming languages (e.g., first.py,.rs,.go,.java,.rb,.kt,.swiftor similar source file) without a corresponding update to.coderabbit.yamlunderreviews.path_instructions,tools:, orcode_generation.docstrings.path_instructions
Warn if a PR introduces new linter/formatter config files (e.g.,.flake8,pylintrc,.pylintrc,pyproject.tomlwith[tool.ruff]/[tool.pylint],.eslintrc*,.stylelintrc*,biome.json,.rubocop.yml) without corresponding tool being enabled or disabled undertools:in.coderabbit.yaml
Warn if a PR introduces new file extensions not covered by existing path_instructions in.coderabbit.yaml(e.g.,.wgsl,.proto,.rego,.tf,.prisma) without updatingreviews.path_instructions
Warn if a PR introduces new CI/tooling configuration files (e.g.,.github/workflows/files using third-party Actions or newdependabot.ymlsections) without updating corresponding path_instructions in.coderabbit.yaml
Files:
.coderabbit.yaml
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Python source files. Review for:
- PEP 8 compliance (enforced by ruff; focus on logic, not style).
- Type annotation coverage — prefer typed signatures for public functions.
- Exception handling: avoid bare
except:clauses; always catch specific exceptions.- Resource management: use
withstatements for file/socket/connection handling.- No mutable default arguments (e.g.
def foo(x=[]):).- When suggesting how to run Python tooling, prefer the
./img2numDocker wrapper
rather than directpython/ruff/pylintinvocations.
Files:
example-apps/console-py/main.pypackages/py/img2num/api.py
bindings/**
⚙️ CodeRabbit configuration file
bindings/**: Language bindings for the Img2Num library. Ensure the public API surface matches
the core C/C++ headers and that error propagation is handled correctly for each
binding language. Also ensure the relevant docstrings are present to enable
auto-generation of documentation.
Files:
bindings/py/src/img2num_pybind.cppbindings/c/src/cimg2num.cppbindings/c/include/cimg2num.hbindings/js/src/wasm_wrapper.cbindings/c/CMakeLists.txtbindings/py/CMakeLists.txt
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{js,ts,jsx,tsx}: Use 2-space indentation for JavaScript and TypeScript files
Maintain 200 character maximum line length for JavaScript/TypeScript files
Files:
packages/js/safeWasmWrappers.js
{CMakeLists.txt,**/*.cmake}
📄 CodeRabbit inference engine (.editorconfig)
{CMakeLists.txt,**/*.cmake}: Use 2-space indentation for CMake configuration files
Maintain 120 character maximum line length for CMake files
Files:
CMakeLists.txt
🧠 Learnings (13)
📚 Learning: 2026-02-25T21:24:34.055Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 272
File: core/include/internal/bilateral_filter_gpu.h:108-109
Timestamp: 2026-02-25T21:24:34.055Z
Learning: In WGSL shaders, .rgb swizzle on a vec4 returns the first three components regardless of color space. Do not assume data is RGB color space when the texture contains non-RGB data (e.g., CIELAB L, A, B). If LAB data is stored, treat and compute distances in LAB space, not RGB. Clearly document shader code paths that rely on specific color spaces and prefer explicit conversions or comments when using swizzled components with non-RGB textures. Apply this guidance to WGSL shader files across the codebase (not just this header) when handling color data or color-like channels.
Applied to files:
core/include/internal/bezier.hcore/include/internal/LABPixel.hcore/include/internal/douglas_peucker.hcore/include/internal/shared_contours.hcore/include/internal/RGBPixel.hcore/include/internal/gpu.hcore/include/internal/LABAPixel.hcore/include/img2num.hcore/include/internal/RGBAPixel.hcore/include/internal/contours.hcore/include/internal/graph.hbindings/c/include/cimg2num.h
📚 Learning: 2026-04-09T19:05:40.514Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 302
File: core/include/internal/gpu.h:203-216
Timestamp: 2026-04-09T19:05:40.514Z
Learning: In Img2Num’s internal GPU initialization code, if querying adapter limits fails (e.g., `adapter.GetLimits(...)` returns `false`), treat this as a GPU initialization failure and route execution to the CPU fallback path. Do not continue with default limits in this case; the failure should cause the same fallback behavior as other GPU init errors.
Applied to files:
core/include/internal/bezier.hcore/include/internal/LABPixel.hcore/include/internal/douglas_peucker.hcore/include/internal/shared_contours.hcore/include/internal/RGBPixel.hcore/include/internal/gpu.hcore/include/internal/LABAPixel.hcore/include/internal/RGBAPixel.hcore/include/internal/contours.hcore/include/internal/graph.h
📚 Learning: 2026-05-01T22:50:11.527Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 339
File: release-please-config.json:18-47
Timestamp: 2026-05-01T22:50:11.527Z
Learning: In this repo, release-please-action v4 preserves '/' verbatim in slash-containing path-based package keys when emitting GitHub Actions output names (e.g., `bindings/c--release_created`). When referencing these step outputs in `job.outputs` (and other expressions), use bracket notation with the exact output name: `${{ steps.release.outputs['bindings/c--release_created'] }}` rather than dot notation. If needed, map the complex step output to a clean job-level output alias so downstream jobs can use dot notation via that alias.
Applied to files:
.github/workflows/pr-target-check.yml.github/workflows/release.yml
📚 Learning: 2026-05-19T17:30:09.565Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 375
File: .github/workflows/cmake-build.yml:86-88
Timestamp: 2026-05-19T17:30:09.565Z
Learning: In Ryan-Millard/Img2Num CI/workflow YAMLs, any `uv sync` command used for the Python package build must include `--no-build-isolation` (do not remove it). If you need deterministic dependency installs for CI, you may add `--frozen` alongside it (e.g., `uv sync --frozen --no-build-isolation`), and it should not conflict with the repo’s build setup.
Applied to files:
.github/workflows/pr-target-check.yml.github/workflows/release.yml
📚 Learning: 2026-04-02T18:39:20.647Z
Learnt from: Krasner
Repo: Ryan-Millard/Img2Num PR: 290
File: core/src/internal/graph.cpp:138-208
Timestamp: 2026-04-02T18:39:20.647Z
Learning: In Ryan-Millard/Img2Num, `add_edge_pixel()` on a `Node` is intentionally designed to allow dual ownership of boundary pixels (the same pixel may appear in the node’s `m_pixels` and also in a neighboring node’s `m_edge_pixels` at the same time). This is required to avoid the SVG “1 pixel boundary problem” (a 1-pixel boundary gap artifact). Treat this as correct behavior: do not review it as a bug, memory/logic misuse, or an invalid data-structure invariant. Note that edge pixels are used only to expand the contour/binary representation for boundary tracing and do not affect node properties such as color.
Applied to files:
.coderabbit.yaml
📚 Learning: 2026-04-11T22:21:32.583Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 308
File: Dockerfile.dev:96-97
Timestamp: 2026-04-11T22:21:32.583Z
Learning: In this repo’s development Docker image (Dockerfile.dev), keep the root user as an intentional, acceptable design for dev workflows. Rationale to preserve: pnpm is installed under /root/.local/share/pnpm, /usr/src/app is a host-mounted volume where UID mismatches can cause permission issues, and dev tooling needs root-level flexibility. Do not suggest switching to a non-root USER for Dockerfile.dev unless you can provide a careful, qualified alternative that avoids pnpm path and host-volume permission problems.
Applied to files:
.coderabbit.yaml
📚 Learning: 2026-04-25T22:11:19.587Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 327
File: docs/package.json:132-132
Timestamp: 2026-04-25T22:11:19.587Z
Learning: In Ryan-Millard/Img2Num, treat `docusaurus/faster` as a required runtime dependency: ensure it is present in `docs/package.json`. This is needed for Docusaurus 3.10 builds because the project’s `docs/docusaurus.config.js` uses `future: { v4: true }`, enabling `fasterByDefault`, which requires `docusaurus/faster` to be installed; it should not be removed or considered optional for performance in this repo.
Applied to files:
.coderabbit.yaml
📚 Learning: 2025-12-31T17:46:54.476Z
Learnt from: fransafu
Repo: Ryan-Millard/Img2Num PR: 176
File: src/wasm/modules/image/src/bilateral_filter.cpp:98-101
Timestamp: 2025-12-31T17:46:54.476Z
Learning: In bilateral_filter.cpp (src/wasm/modules/image/src/bilateral_filter.cpp), rely on the fact that the center pixel contributes a weight of exactly 1.0 to both spatial and range components (exp(0) = 1) so the normalization sum cannot be zero. This implies you do not need explicit guards against division by zero for normalization in this implementation.
Applied to files:
core/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppbindings/py/src/img2num_pybind.cppcore/src/internal/kmeans_gpu.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_utils.cppcore/src/internal/douglas_peucker.cppcore/src/internal/contours.cppcore/src/internal/bezier.cppbindings/c/src/cimg2num.cppcore/src/internal/shared_contours.cppcore/src/internal/graph.cpp
📚 Learning: 2026-01-06T04:56:57.269Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/kmeans.cpp:128-134
Timestamp: 2026-01-06T04:56:57.269Z
Learning: In C++ sources compiled for WASM with Emscripten/Clang, designated initializers (e.g., RGBXY{.r = ..., .g = ...}) are allowed as a C++17 extension. When reviewing code that relies on designated initializers, verify that the target toolchain enables CXX_STANDARD 17 or higher and that the build system (CMake/emsdk) uses Emscripten with a compatible clang. If not, avoid such initializers or provide portable alternatives.
Applied to files:
core/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppbindings/py/src/img2num_pybind.cppcore/src/internal/kmeans_gpu.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_utils.cppcore/src/internal/douglas_peucker.cppcore/src/internal/contours.cppcore/src/internal/bezier.cppbindings/c/src/cimg2num.cppcore/src/internal/shared_contours.cppcore/src/internal/graph.cpp
📚 Learning: 2026-01-06T21:06:24.476Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/cielab.cpp:43-57
Timestamp: 2026-01-06T21:06:24.476Z
Learning: In the Img2Num project, prefer recommending and using the provided docker/script wrappers (e.g., ./img2num format-wasm, ./img2num clean-wasm) instead of invoking local tools directly (e.g., clang-format -i). This reduces dependency requirements for users and ensures consistent tooling across environments. Apply this guidance to C++ source files under the project when reviewing changes.
Applied to files:
core/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppbindings/py/src/img2num_pybind.cppcore/src/internal/kmeans_gpu.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_utils.cppcore/src/internal/douglas_peucker.cppcore/src/internal/contours.cppcore/src/internal/bezier.cppbindings/c/src/cimg2num.cppcore/src/internal/shared_contours.cppcore/src/internal/graph.cpp
📚 Learning: 2026-01-19T00:02:34.957Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 234
File: src/wasm/modules/image/src/kmeans.cpp:79-81
Timestamp: 2026-01-19T00:02:34.957Z
Learning: In C++ multithreading contexts, prefer unsigned types for thread-count-like parameters (e.g., n_threads) to prevent negative values. Validate that the value is at least 1 before any division to avoid divide-by-zero at runtime. At the start of functions handling such values, create a safe count like: const unsigned int thread_count{std::max(1u, n_threads)}; This ensures non-negative, non-zero usage for divisions and related arithmetic. Apply this pattern to similar parameters across C++ modules, not just the specific file.
Applied to files:
core/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppbindings/py/src/img2num_pybind.cppcore/src/internal/kmeans_gpu.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_utils.cppcore/src/internal/douglas_peucker.cppcore/src/internal/contours.cppcore/src/internal/bezier.cppbindings/c/src/cimg2num.cppcore/src/internal/shared_contours.cppcore/src/internal/graph.cpp
📚 Learning: 2026-02-25T21:24:19.036Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 272
File: core/src/internal/bilateral_filter_gpu.cpp:171-185
Timestamp: 2026-02-25T21:24:19.036Z
Learning: Adopt brace initialization (e.g., Type var{}) over copy initialization (e.g., Type var = {}) for zero-initialization in C++ across the repository. This should apply to variables, arrays, and structs in most C++ files (e.g., core/src/internal/bilateral_filter_gpu.cpp). It improves safety by avoiding narrowing conversions and makes initialization intent explicit.
Applied to files:
core/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppbindings/py/src/img2num_pybind.cppcore/src/internal/kmeans_gpu.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_utils.cppcore/src/internal/douglas_peucker.cppcore/src/internal/contours.cppcore/src/internal/bezier.cppbindings/c/src/cimg2num.cppcore/src/internal/shared_contours.cppcore/src/internal/graph.cpp
📚 Learning: 2026-04-27T15:40:33.329Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 333
File: bindings/c/src/cimg2num.cpp:0-0
Timestamp: 2026-04-27T15:40:33.329Z
Learning: In the Ryan-Millard/Img2Num C bindings (e.g., `bindings/c/src/cimg2num.cpp`) that expose `extern "C"` functions, do not allow C++ exceptions (including `std::bad_alloc` and exceptions thrown from lambdas used internally) to propagate out of these C binding functions. Use the C-API failure idiom instead: catch exceptions internally as needed, clear/record error state via the existing error mechanism, and return `nullptr` (or the documented sentinel return value) on failure so the C caller can detect errors by checking the return value.
Applied to files:
bindings/c/src/cimg2num.cpp
🪛 Cppcheck (2.21.0)
core/src/internal/douglas_peucker.cpp
[style] 104-104: The function 'dp_curve_reduction' is never used.
(unusedFunction)
core/src/internal/contours.cpp
[style] 834-834: The function 'coupled_smooth_junctions' is never used.
(unusedFunction)
core/src/internal/shared_contours.cpp
[style] 61-61: The function 'build_shared_loops' is never used.
(unusedFunction)
bindings/js/src/wasm_wrapper.c
[style] 44-44: The function 'image_to_svg' is never used.
(unusedFunction)
🪛 markdownlint-cli2 (0.22.1)
.github/PULL_REQUEST_TEMPLATE.md
[warning] 11-11: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🪛 zizmor (1.25.2)
.github/workflows/pr-target-check.yml
[warning] 21-21: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
[warning] 9-12: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
[info] 24-24: action functionality is already included by the runner (superfluous-actions): use gh pr comment or gh issue comment in a script step
(superfluous-actions)
.github/workflows/release.yml
[warning] 3-7: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
[info] 41-41: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 46-46: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 53-53: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[info] 83-83: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 84-84: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 85-85: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 87-87: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 88-88: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 89-89: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 102-102: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 106-106: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 154-157: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 151-151: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
[info] 144-144: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[info] 136-136: action functionality is already included by the runner (superfluous-actions): use gh release in a script step
(superfluous-actions)
[warning] 189-189: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 181-181: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
[info] 195-195: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[info] 175-175: action functionality is already included by the runner (superfluous-actions): use gh release in a script step
(superfluous-actions)
[info] 299-299: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[info] 277-277: action functionality is already included by the runner (superfluous-actions): use gh release in a script step
(superfluous-actions)
🔇 Additional comments (47)
core/src/internal/shared_contours.cpp (11)
1-16: LGTM!
17-38: LGTM!
40-52: LGTM!
54-58: LGTM!
60-93: LGTM!
95-114: LGTM!
116-163: LGTM!
165-198: LGTM!
200-221: LGTM!
223-274: LGTM!
276-333: LGTM!core/src/internal/douglas_peucker.cpp (3)
1-17: LGTM!
19-84: LGTM!
118-158: LGTM!core/src/internal/contours.cpp (1)
732-732: LGTM!core/src/internal/graph.cpp (1)
4-10: LGTM!Also applies to: 221-307, 309-397, 399-448
core/src/internal/labels_to_svg.cpp (1)
203-206: LGTM!Also applies to: 224-224
core/src/internal/image_to_svg.cpp (1)
26-28: LGTM!core/src/internal/image_utils.cpp (1)
3-8: LGTM!Also applies to: 142-144
core/src/internal/kmeans_gpu.cpp (1)
68-68: LGTM!example-apps/console-cpp/main.cpp (1)
70-72: LGTM!example-apps/console-py/main.py (1)
26-34: LGTM!bindings/c/src/cimg2num.cpp (1)
10-24: LGTM!Also applies to: 26-40, 91-104
bindings/js/src/wasm_wrapper.c (1)
38-42: LGTM!Also applies to: 44-47, 56-56
bindings/py/src/img2num_pybind.cpp (1)
124-137: LGTM!Also applies to: 186-187, 198-198, 208-208
packages/js/safeWasmWrappers.js (1)
187-198: LGTM!Also applies to: 225-225, 234-237
example-apps/console-c/main.c (1)
75-75: LGTM!core/CMakeLists.txt (1)
41-42: LGTM!bindings/c/CMakeLists.txt (1)
21-22: LGTM!bindings/py/CMakeLists.txt (1)
19-23: LGTM!example-apps/console-c/CMakeLists.txt (1)
4-4: LGTM!Also applies to: 13-28
example-apps/console-cpp/CMakeLists.txt (1)
4-25: LGTM!pyproject.toml (1)
42-42: LGTM!Also applies to: 90-91
.coderabbit.yaml (1)
29-30: LGTM!.github/ISSUE_TEMPLATE/bug_report.yml (1)
6-77: LGTM!.github/ISSUE_TEMPLATE/feature_request.yml (1)
1-43: LGTM!.github/PULL_REQUEST_TEMPLATE.md (1)
11-25: LGTM!core/include/img2num.h (1)
45-49: LGTM!core/include/internal/LABAPixel.h (1)
11-11: LGTM!core/include/internal/LABPixel.h (1)
19-19: LGTM!core/include/internal/RGBAPixel.h (1)
11-11: LGTM!core/include/internal/RGBPixel.h (1)
14-14: LGTM!core/include/internal/shared_contours.h (1)
11-25: LGTM!core/include/internal/douglas_peucker.h (1)
9-29: LGTM!core/include/internal/bezier.h (1)
11-18: LGTM!core/include/internal/graph.h (1)
104-104: LGTM!core/include/internal/gpu.h (1)
232-235: LGTM!
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (20)
.github/workflows/pr-target-check.yml (1)
19-24:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
fd pr-target-check.yml --type fRepository: Ryan-Millard/Img2Num
Length of output: 46
🏁 Script executed:
cat -n .github/workflows/pr-target-check.ymlRepository: Ryan-Millard/Img2Num
Length of output: 1673
🌐 Web query:
peter-evans/create-or-update-comment action permissions issue-number vs pull-request-number💡 Result:
In the peter-evans/create-or-update-comment action, there is no separate pull-request-number input; the issue-number input is used for both issues and pull requests [1][2]. In GitHub's API, pull requests are a type of issue, so the action's issue-number input accepts the pull request number when targeting a pull request [1][3]. When configuring your workflow, you can retrieve the correct number dynamically based on the event: - For pull_request events: Use ${{ github.event.pull_request.number }} [4][2]. - For issue_comment events: Use ${{ github.event.issue.number }} [4]. A common pattern for cross-event workflows is to use a fallback or conditional expression, such as ${{ github.event.pull_request.number || github.event.issue.number }} [4]. If you need to perform an update rather than creating a new comment, you must provide a comment-id (often retrieved via the peter-evans/find-comment action) in addition to the issue-number [5][3].
Citations:
- 1: https://github.com/peter-evans/create-or-update-comment
- 2: https://cicube.io/workflow-hub/peter-evans-create-or-update-comment/
- 3: https://github.com/peter-evans/create-or-update-comment/
- 4: https://stackoverflow.com/questions/59077079/how-to-get-pull-request-number-within-github-actions-workflow
- 5: peter-evans/create-or-update-comment#364
🌐 Web query:
peter-evans/create-or-update-comment GitHub permissions issue-number pull-request-number required scope💡 Result:
To use the peter-evans/create-or-update-comment GitHub Action, you must provide appropriate write permissions for your GITHUB_TOKEN within your workflow file [1][2]. The required scopes are issues: write and pull-requests: write [1][3]. Setting the correct permissions is necessary to resolve the common Resource not accessible by integration error [4][5]. You should explicitly define these permissions in your workflow configuration as follows [5]: permissions: issues: write pull-requests: write Regarding inputs: - issue-number: This is a required input to specify the number of the issue or pull request where the comment should be created or updated [1][3]. - comment-id: This input is used when you intend to update an existing comment rather than create a new one [1][3]. Additionally, note that in public repositories, this action may face token restrictions when triggered by fork events (e.g., in pull_request workflows), which can prevent it from commenting [1][2]. For these scenarios, it is recommended to use the pull_request_target event instead [1][6].
Citations:
- 1: https://github.com/peter-evans/create-or-update-comment
- 2: https://github.com/marketplace/actions/create-or-update-comment
- 3: https://github.com/peter-evans/create-or-update-comment/
- 4: peter-evans/create-or-update-comment#84
- 5: peter-evans/create-or-update-comment#395
- 6: https://github.com/peter-evans/create-or-update-comment/blob/af66e5eb29027c7ba4cfa34c4a0203d34bc1431e/README.md
🏁 Script executed:
# Search for any documentation or examples of this action's permission requirements rg "peter-evans/create-or-update-comment" --type yml --context 5Repository: Ryan-Millard/Img2Num
Length of output: 92
Add
issues: writepermission for the warning-comment step.At line 21, the job only grants
pull-requests: write, but the peter-evans/create-or-update-comment action requires bothissues: writeandpull-requests: writepermissions. The action uses the issues API (issue-numberparameter), so the step will fail with insufficient token scope without both scopes.🔧 Suggested fix
permissions: - # Required to post warning comment - pull-requests: write + # Required to post PR conversation comment (issues API) + issues: write + pull-requests: write🧰 Tools
🪛 zizmor (1.25.2)
[warning] 21-21: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
[info] 24-24: action functionality is already included by the runner (superfluous-actions): use
gh pr commentorgh issue commentin a script step(superfluous-actions)
🤖 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 @.github/workflows/pr-target-check.yml around lines 19 - 24, The permissions section in the workflow job is missing the required `issues: write` permission for the peter-evans/create-or-update-comment action. Add `issues: write` permission alongside the existing `pull-requests: write` permission in the permissions block so that the create-or-update-comment step has the necessary API scopes to execute successfully.Source: Coding guidelines
.github/workflows/release.yml (4)
5-7: 🧹 Nitpick | 🔵 Trivial
Add workflow-level concurrency for release runs.
A branch-level concurrency group helps prevent overlapping release pipelines from racing on tags/releases/artifacts.
Suggested addition
concurrency: group: release-${{ github.ref }} cancel-in-progress: true🧰 Tools
🪛 zizmor (1.25.2)
[warning] 3-7: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 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 @.github/workflows/release.yml around lines 5 - 7, The release workflow lacks concurrency configuration at the workflow level, which allows multiple release pipelines to run simultaneously and potentially race on tags and artifacts. Add a concurrency block to the workflow after the trigger configuration (the on section with branches: [main, dev] for push and pull_request) with a group identifier based on the git reference (github.ref) and set cancel-in-progress to true so that any in-progress release runs are cancelled when a new one starts on the same branch.Source: Linters/SAST tools
135-142:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign draft-release gating with artifact packaging gating.
This step runs for dry-runs even when packaging steps are skipped, so
env.ARCHIVEmay be unset and the release step can fail or publish without artifacts.Suggested fix
- - name: Package (Linux/macOS) - if: ${{ steps.component.outputs.created == 'true' && runner.os != 'Windows' }} + - name: Package (Linux/macOS) + if: ${{ (steps.component.outputs.created == 'true' || needs.release-please.outputs.is_dry_run == 'true') && runner.os != 'Windows' }} - - name: Package (Windows) - if: ${{ steps.component.outputs.created == 'true' && runner.os == 'Windows' }} + - name: Package (Windows) + if: ${{ (steps.component.outputs.created == 'true' || needs.release-please.outputs.is_dry_run == 'true') && runner.os == 'Windows' }}🧰 Tools
🪛 zizmor (1.25.2)
[info] 136-136: action functionality is already included by the runner (superfluous-actions): use
gh releasein a script step(superfluous-actions)
🤖 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 @.github/workflows/release.yml around lines 135 - 142, The softprops/action-gh-release step uses the if condition that checks if the component was created or if it's a dry run, but this doesn't align with the conditions that gate the artifact packaging steps that set env.ARCHIVE. This causes the release step to potentially run without artifacts being packaged. Identify the actual conditions under which the artifact packaging steps execute (the steps that set env.ARCHIVE), and update the if condition in the softprops/action-gh-release step to match those same conditions. This ensures the release step only runs when artifacts have been prepared and env.ARCHIVE is properly set.
154-157:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable persisted Git credentials in JS checkout.
Set
persist-credentials: falseto reduce token exposure during downstream build/publish steps.As per coding guidelines, GitHub workflows should follow least-privilege secret handling.
Suggested fix
- uses: actions/checkout@900f2210b1d28bbbd0bd22d17926b9e224e8f231 with: ref: ${{ needs.release-please.outputs.is_dry_run == 'true' && github.sha || needs.release-please.outputs.js_tag }} submodules: true + persist-credentials: false🧰 Tools
🪛 zizmor (1.25.2)
[warning] 154-157: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/release.yml around lines 154 - 157, The actions/checkout action in the release workflow is missing the persist-credentials setting, which leaves Git credentials exposed during downstream build and publish steps. Add persist-credentials: false to the with section of the actions/checkout@900f2210b1d28bbbd0bd22d17926b9e224e8f231 action to limit credential exposure and follow least-privilege secret handling practices.Source: Coding guidelines
276-297:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the Python dry-run publish branch reachable.
These dry-run/TestPyPI steps are inside a job that still only runs when
py_created == 'true', so the dry-run path is skipped in non-release dry-run executions.Suggested fix (job-level gate)
- publish-py: - if: ${{ needs.release-please.outputs.py_created == 'true' }} + publish-py: + if: ${{ needs.release-please.outputs.py_created == 'true' || needs.release-please.outputs.is_dry_run == 'true' }}🧰 Tools
🪛 zizmor (1.25.2)
[info] 277-277: action functionality is already included by the runner (superfluous-actions): use
gh releasein a script step(superfluous-actions)
🤖 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 @.github/workflows/release.yml around lines 276 - 297, The pypa/gh-action-pypi-publish steps for both production PyPI and TestPyPI publishing are contained within a job that only runs when py_created == 'true', which prevents the dry-run path from being reachable. Update the job-level conditional (the if condition on the job definition) to also allow execution when a dry-run is being performed, so that the step-level conditionals can properly route to either production PyPI or TestPyPI based on the is_dry_run flag.bindings/c/include/cimg2num.h (1)
45-46:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect changed C API surface rg -nP 'typedef struct img2num_ImageToSvgConfig|img2num_labels_to_svg\s*\(' \ bindings/c/include/cimg2num.h bindings/c/src/cimg2num.cpp # Inspect versioning/ABI metadata in build config rg -nP 'project\s*\(|SOVERSION|VERSION|IMG2NUM_VERSION|CPACK_PACKAGE_VERSION' \ CMakeLists.txt core/CMakeLists.txt bindings/c/CMakeLists.txtRepository: Ryan-Millard/Img2Num
Length of output: 893
🏁 Script executed:
#!/bin/bash # Get full struct definition context around min_thickness sed -n '20,50p' bindings/c/include/cimg2num.h # Get full function signature context sed -n '85,95p' bindings/c/include/cimg2num.h sed -n '89,110p' bindings/c/src/cimg2num.cpp # Check for tests related to min_thickness rg -n 'min_thickness' bindings/c/ --include='*.c' --include='*.cpp' --include='*.h' --include='*.hpp' # Search for CHANGELOG or release notes fd -e md -e txt | xargs rg -l 'CHANGELOG|BREAKING|VERSION' 2>/dev/null | head -10Repository: Ryan-Millard/Img2Num
Length of output: 3291
🏁 Script executed:
#!/bin/bash # Check if there are C API tests fd -n 'test.*\.c$|.*_test\.c$' bindings/c/ # Look for test structure or test files find bindings/c -type f \( -name '*test*' -o -name '*spec*' \) 2>/dev/null # Check CMakeLists for test configuration rg -n 'add_test|enable_testing|gtest|catch' bindings/c/CMakeLists.txtRepository: Ryan-Millard/Img2Num
Length of output: 295
🏁 Script executed:
#!/bin/bash # Check for C binding tests - try different patterns find bindings/c -type f \( -name '*test*' -o -name '*spec*' \) # List all files in bindings/c to understand structure find bindings/c -type f | head -20 # Check for test patterns in CMakeLists grep -n 'test\|Test\|TEST' bindings/c/CMakeLists.txt || echo "No test patterns found"Repository: Ryan-Millard/Img2Num
Length of output: 386
🏁 Script executed:
#!/bin/bash # Check the CHANGELOG for versioning context cat bindings/c/CHANGELOG.md # Also check core CHANGELOG for ABI versioning policy head -50 core/CHANGELOG.mdRepository: Ryan-Millard/Img2Num
Length of output: 4693
🏁 Script executed:
#!/bin/bash # Search for min_thickness in the codebase to see where it's used rg 'min_thickness' bindings/c/ # Also search in core to verify the C++ API rg 'min_thickness' core/Repository: Ryan-Millard/Img2Num
Length of output: 1517
Add test coverage for the C binding
min_thicknessfeature.Per the contribution guidelines, new features must include tests. The
min_thicknessparameter is exposed in the C API (img2num_labels_to_svgsignature andimg2num_ImageToSvgConfigstruct) but no corresponding tests exist inbindings/c/. Add tests that verify the parameter is correctly passed through the C binding to the core implementation and produces expected SVG output with variousmin_thicknessvalues.🤖 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 `@bindings/c/include/cimg2num.h` around lines 45 - 46, The min_thickness parameter has been added to the img2num_ImageToSvgConfig struct and img2num_labels_to_svg function in the C API but lacks test coverage. Add test cases in the bindings/c/ test directory that verify the min_thickness parameter is correctly passed through the C binding to the core implementation. Create tests that validate SVG output behavior with various min_thickness values (including edge cases like zero, small, and large values) to ensure regions are included or excluded from the SVG based on the thickness threshold.CMakeLists.txt (2)
22-23:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not default full cache-value dumping to enabled.
The current default logs all cache entries and values, which can leak sensitive
-D...inputs in CI logs.Suggested fix
-option(IMG2NUM_DEBUG_CACHE_VARIABLES_DUMP "Dump CMake environment variables in Debug mode" ON) +option(IMG2NUM_DEBUG_CACHE_VARIABLES_DUMP "Dump CMake environment variables in Debug mode" OFF)- foreach(_var ${_vars}) - if(NOT _var MATCHES "^IMG2NUM") - message(STATUS "${COLOR_YELLOW}${_var}=${${_var}}${COLOR_RESET}") - endif() - endforeach() + # Avoid printing all non-IMG2NUM cache values to prevent accidental secret leakage.Also applies to: 56-60
🤖 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 `@CMakeLists.txt` around lines 22 - 23, Change the default value of the IMG2NUM_DEBUG_CACHE_VARIABLES_DUMP option from ON to OFF to prevent sensitive information (such as cache variables and inputs) from being logged and potentially leaked in CI logs. Additionally, review lines 56-60 for similar cache variable dumping options that may have the same issue and apply the same fix by changing their defaults from ON to OFF.
72-82:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix mutually exclusive binding selection logic.
This
elseifchain prevents valid combinations. WithIMG2NUM_BUILD_C=ON, theEMSCRIPTENbranch is skipped, sobindings/jswon’t be added even for WASM builds.Suggested fix
-if(IMG2NUM_BUILD_PYTHON) - # Python depends only on core - add_subdirectory(bindings/py) -elseif(IMG2NUM_BUILD_C) - # C depends only on core - add_subdirectory(bindings/c) -elseif(EMSCRIPTEN) - # WebAssembly relies on C - add_subdirectory(bindings/c) - add_subdirectory(bindings/js) -endif() +if(IMG2NUM_BUILD_PYTHON) + add_subdirectory(bindings/py) +endif() + +if(IMG2NUM_BUILD_C OR EMSCRIPTEN) + add_subdirectory(bindings/c) +endif() + +if(EMSCRIPTEN) + add_subdirectory(bindings/js) +endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if(IMG2NUM_BUILD_PYTHON) add_subdirectory(bindings/py) endif() if(IMG2NUM_BUILD_C OR EMSCRIPTEN) add_subdirectory(bindings/c) endif() if(EMSCRIPTEN) add_subdirectory(bindings/js) endif()🤖 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 `@CMakeLists.txt` around lines 72 - 82, The conditional logic in the CMakeLists.txt file uses elseif statements that make the binding selection mutually exclusive, preventing valid combinations like building both C and JavaScript bindings for WASM. Restructure this block to allow independent conditions for each binding type instead of using elseif chains. Keep IMG2NUM_BUILD_PYTHON as mutually exclusive if needed, but convert the IMG2NUM_BUILD_C and EMSCRIPTEN conditions to separate if statements (or restructure appropriately) so that when both IMG2NUM_BUILD_C and EMSCRIPTEN are true, both the C bindings and JS bindings can be added together.core/include/img2num.h (1)
83-86:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify declaration/definition mismatch and inspect call sites. rg -nP 'std::string\s+labels_to_svg\s*\(' core/include/img2num.h core/src/internal/labels_to_svg.cpp # Review all call sites to see whether any still rely on the 5-argument form. rg -nP --type=cpp '\blabels_to_svg\s*\([^)]*\)' core bindings example-appsRepository: Ryan-Millard/Img2Num
Length of output: 428
🏁 Script executed:
sed -n '80,90p' core/include/img2num.hRepository: Ryan-Millard/Img2Num
Length of output: 428
🏁 Script executed:
sed -n '200,215p' core/src/internal/labels_to_svg.cppRepository: Ryan-Millard/Img2Num
Length of output: 723
Fix API compatibility mismatch: add default argument to
labels_to_svgdeclaration in header.The header declaration (line 83–86) is missing the default value for the
min_thicknessparameter, while the implementation incore/src/internal/labels_to_svg.cppincludesconst int min_thickness = 0. This declaration/implementation mismatch forces external callers to provide all 6 arguments, breaking source compatibility. Add= 0to the header declaration to match the implementation.🤖 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 `@core/include/img2num.h` around lines 83 - 86, The function declaration for labels_to_svg in the header file is missing a default argument value for the min_thickness parameter, while the implementation in labels_to_svg.cpp includes a default value of 0. This mismatch breaks source compatibility by requiring callers to provide all arguments. Add the default value = 0 to the min_thickness parameter in the labels_to_svg function declaration in the header file to match the implementation and restore API compatibility.core/include/internal/contours.h (1)
64-67:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify whether `junctions` is mutated in implementation before switching to const-ref. rg -n -C4 'coupled_smooth_junctions\s*\(' core/include/internal/contours.h core/src/internal/contours.cpp rg -n -C3 '\bjunctions\b' core/src/internal/contours.cppRepository: Ryan-Millard/Img2Num
Length of output: 2286
🏁 Script executed:
rg -n 'coupled_smooth_junctions\s*\(' --type cpp --type h -A 2 -B 2Repository: Ryan-Millard/Img2Num
Length of output: 777
🏁 Script executed:
rg -n 'coupled_smooth_junctions\s*\(' --type cpp --type h | grep -v '^\s*(void|static)' | grep -v 'def\|declaration' rg 'coupled_smooth_junctions' --type cpp --type h -B 2 -A 2 | grep -v '^--$' | grep -v 'void coupled_smooth_junctions'Repository: Ryan-Millard/Img2Num
Length of output: 746
🏁 Script executed:
rg 'coupled_smooth_junctions\s*\(' --type cpp --type h -g '!*.h' -g '!contours.h' | head -20 rg 'coupled_smooth_junctions' core --type cpp | grep -v 'void coupled_smooth_junctions'Repository: Ryan-Millard/Img2Num
Length of output: 126
🏁 Script executed:
rg '\bcoupled_smooth_junctions\b' --type cpp --type h | grep -v 'void coupled_smooth_junctions' | grep -v '^\s*//'Repository: Ryan-Millard/Img2Num
Length of output: 46
🏁 Script executed:
git grep -n 'coupled_smooth_junctions' 2>/dev/null || rg 'coupled_smooth_junctions' . --type-list | head -20 find . -type f \( -name '*.cpp' -o -name '*.h' -o -name '*.hpp' -o -name '*.js' -o -name '*.ts' -o -name '*.py' \) -exec grep -l 'coupled_smooth_junctions' {} \;Repository: Ryan-Millard/Img2Num
Length of output: 150
🏁 Script executed:
rg 'coupled_smooth_junctions' core/src/internal/contours.cpp -nRepository: Ryan-Millard/Img2Num
Length of output: 99
🏁 Script executed:
grep -n 'coupled_smooth_junctions' core/include/internal/contours.h grep -n 'coupled_smooth_junctions' core/include/img2num.hRepository: Ryan-Millard/Img2Num
Length of output: 98
Avoid copying the junction mask on every call.
junctionsis passed by value, which copies the full mask buffer per invocation. This is expensive for image-sized data and unnecessary since the parameter is never mutated.💡 Suggested signature change
void coupled_smooth_junctions( - std::vector<std::vector<Point>>& contours, Rect bounds, std::vector<uint8_t> junctions, + std::vector<std::vector<Point>>& contours, Rect bounds, const std::vector<uint8_t>& junctions, int width );Apply the same change to both the declaration in
core/include/internal/contours.hand the definition incore/src/internal/contours.cpp, then run./img2num format-cppto ensure compliance with the clang-format style.🤖 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 `@core/include/internal/contours.h` around lines 64 - 67, The coupled_smooth_junctions function passes the junctions parameter by value, causing unnecessary copying of the image-sized mask buffer on every invocation. Since the junctions parameter is never mutated within the function, change it to be passed as a const reference instead. Apply this parameter modification to both the function declaration in contours.h and the function definition in contours.cpp, then run ./img2num format-cpp to ensure the changes comply with the clang-format style guide.core/include/internal/graph.h (1)
52-56:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check getPixel callsites and whether upstream enforces img.size() == w*h. rg -n -C4 '\bgetPixel\s*\(' core/include/internal/graph.h core/src/internal/graph.cpp rg -n -C4 'analyzeJunctions|size\(\)|w\s*\*\s*h|skeleton|skel' core/src/internal/graph.cppRepository: Ryan-Millard/Img2Num
Length of output: 3340
🏁 Script executed:
# Find all calls to analyzeJunctions to check how skel is constructed rg -n 'analyzeJunctions' core/ --type cpp --type hpp -B5 -A2Repository: Ryan-Millard/Img2Num
Length of output: 92
🏁 Script executed:
# Check how skeleton images (skel) are typically created to see if size matches w*h rg -n 'skeleton|skel\s*=' core/src/ --type cpp -B2 -A2 | head -100Repository: Ryan-Millard/Img2Num
Length of output: 46
🏁 Script executed:
# Broader search for analyzeJunctions calls rg -n 'analyzeJunctions' core/Repository: Ryan-Millard/Img2Num
Length of output: 315
🏁 Script executed:
# Look for where skel vectors are created and passed to analyzeJunctions rg -n 'analyzeJunctions' . --context 10Repository: Ryan-Millard/Img2Num
Length of output: 2851
🏁 Script executed:
# Check what calls Graph::analyzeJunctions and how skel param is constructed rg -n 'skel.*=' core/ -B3 -A3Repository: Ryan-Millard/Img2Num
Length of output: 1121
🏁 Script executed:
# Find all calls to analyzeJunctions to see how skel is prepared rg -n 'analyzeJunctions' . -B10 | grep -E "(analyzeJunctions|vector|skel|resize|size)" | head -50Repository: Ryan-Millard/Img2Num
Length of output: 556
🏁 Script executed:
# Find the actual call site(s) to analyzeJunctions rg -n '\.analyzeJunctions\(|->analyzeJunctions\(' . -B5 -A3Repository: Ryan-Millard/Img2Num
Length of output: 46
🏁 Script executed:
# Broader search across entire repo rg 'analyzeJunctions' .Repository: Ryan-Millard/Img2Num
Length of output: 312
🏁 Script executed:
# Check if it's a public method by looking at header visibility grep -n 'analyzeJunctions' core/include/internal/graph.h -B3 -A3Repository: Ryan-Millard/Img2Num
Length of output: 389
🏁 Script executed:
# Look for any vector size validation patterns in the codebase rg 'img\.size\(\)|skel\.size\(\)' core/ -A2 -B2Repository: Ryan-Millard/Img2Num
Length of output: 46
🏁 Script executed:
# Final check: see how other similar indexing patterns in core handle vector bounds rg -n '\[.*\s*\*\s*.*\s*\+' core/ -B2 -A2 | head -40Repository: Ryan-Millard/Img2Num
Length of output: 2450
getPixellacks vector size validation and can access out-of-bounds memory.The x/y bounds checks prevent invalid coordinate pairs but do not validate that
img.size()equalsw * h. The computed linear indexy * w + xcan exceed the vector bounds if the caller provides inconsistent dimensions.Suggested fix
inline uint8_t getPixel(const std::vector<uint8_t>& img, int w, int h, int x, int y) { if (x < 0 || x >= w || y < 0 || y >= h) return 0; // Boundary check - return img[y * w + x]; + const size_t idx = static_cast<size_t>(y) * static_cast<size_t>(w) + static_cast<size_t>(x); + if (idx >= img.size()) + return 0; + return img[idx]; }🤖 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 `@core/include/internal/graph.h` around lines 52 - 56, The getPixel function validates the coordinate bounds against width and height parameters but does not verify that the image vector's actual size matches the expected dimensions (w * h). Add a validation check at the beginning of the getPixel function to ensure that img.size() equals w * h before performing the index calculation, returning 0 or handling the error case appropriately if the sizes are inconsistent. This prevents out-of-bounds access when the caller provides mismatched dimensions.core/src/internal/bezier.cpp (1)
163-180:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
fixedvector access before indexing by chain index.Line 179 reads
fixed[i]without verifyingi < fixed.size(). A mismatchedchains/fixedlength causes out-of-bounds access and undefined behavior.🛠️ Suggested fix
void fit_curve_reduction( const std::vector<std::vector<Point>>& chains, const std::vector<std::vector<uint8_t>>& fixed, std::vector<std::vector<QuadBezier>>& results, float tolerance ) { + if (fixed.size() != chains.size()) { + throw std::invalid_argument( + "fit_curve_reduction: fixed.size() must match chains.size()" + ); + } + for (size_t i = 0; i < chains.size(); ++i) { const std::vector<Point>& chain = chains[i]; const int n = static_cast<int>(chain.size()); std::vector<QuadBezier> result; @@ std::vector<int> bounds; bounds.push_back(0); for (int k = 1; k < n - 1; ++k) - if (k < static_cast<int>(fixed[i].size()) && fixed[i][k]) + if (k < static_cast<int>(fixed[i].size()) && fixed[i][k] != 0) bounds.push_back(k); bounds.push_back(n - 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 `@core/src/internal/bezier.cpp` around lines 163 - 180, The code accesses fixed[i] without verifying that i is within bounds of the fixed vector, which causes undefined behavior when chains and fixed have mismatched sizes. Add a bounds check to ensure i < fixed.size() before accessing fixed[i] in the loop where chains are iterated. This check should be applied before or within the condition that currently accesses fixed[i][k] to prevent out-of-bounds access.core/src/internal/contours.cpp (2)
682-691:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd coordinate and index bounds checks before reading
junctions[idx].Lines 689-690 compute an index from float coordinates and dereference without validating bounds. This can read outside
junctionsand crash on out-of-range points.🛠️ Suggested fix
void updateLockedMasks( const std::vector<std::vector<Point>>& contours, std::vector<std::vector<bool>>& locked, - std::vector<uint8_t>& junctions, int width + const std::vector<uint8_t>& junctions, int width ) { + if (width <= 0 || junctions.empty()) { + return; + } + const int height = static_cast<int>(junctions.size() / static_cast<size_t>(width)); + for (size_t c = 0; c < contours.size(); ++c) { for (size_t p = 0; p < contours[c].size(); ++p) { - Point pt = contours[c][p]; - int idx = pt.y * width + pt.x; - if (junctions[idx] > 0) { + const Point pt = contours[c][p]; + const int x = static_cast<int>(std::lround(pt.x)); + const int y = static_cast<int>(std::lround(pt.y)); + if (x < 0 || x >= width || y < 0 || y >= height) { + continue; + } + const size_t idx = + static_cast<size_t>(y) * static_cast<size_t>(width) + static_cast<size_t>(x); + if (idx < junctions.size() && junctions[idx] > 0) { locked[c][p] = true; } } } }🤖 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 `@core/src/internal/contours.cpp` around lines 682 - 691, In the updateLockedMasks function, before accessing junctions[idx] where idx is computed from pt.y * width + pt.x, add bounds validation to ensure the index is non-negative and less than the size of the junctions vector. Insert a conditional check after computing idx that verifies idx is within valid bounds before dereferencing junctions[idx] to prevent out-of-range memory access and potential crashes on invalid point coordinates.
835-840: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Avoid copying the full junction mask in
coupled_smooth_junctions.Line 835 passes
junctionsby value, which copies awidth*heightbuffer. Useconst std::vector<uint8_t>&in both declaration and definition.♻️ Suggested refactor
-void coupled_smooth_junctions( - std::vector<std::vector<Point>>& contours, Rect bounds, std::vector<uint8_t> junctions, - int width -) +void coupled_smooth_junctions( + std::vector<std::vector<Point>>& contours, Rect bounds, + const std::vector<uint8_t>& junctions, int width +)-void updateLockedMasks( - const std::vector<std::vector<Point>>& contours, std::vector<std::vector<bool>>& locked, - std::vector<uint8_t>& junctions, int width -) +void updateLockedMasks( + const std::vector<std::vector<Point>>& contours, std::vector<std::vector<bool>>& locked, + const std::vector<uint8_t>& junctions, int width +)🤖 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 `@core/src/internal/contours.cpp` around lines 835 - 840, The `coupled_smooth_junctions` function passes the `junctions` parameter by value, which unnecessarily copies a `width*height` buffer. Change the parameter type from `std::vector<uint8_t>` to `const std::vector<uint8_t>&` to pass by const reference instead. Apply this change consistently in both the function declaration and its definition to avoid the expensive copy operation.core/src/internal/douglas_peucker.cpp (1)
104-117:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential out-of-bounds access if
fixed.size() < chains.size().The loop iterates over
chains, but accessesfixed[i]without verifying thati < fixed.size(). If a caller passes mismatched vector sizes, this causes undefined behavior.While callers should maintain parallel vectors per the documented contract, a defensive guard would make this more robust.
🛡️ Proposed defensive guard
void dp_curve_reduction( const std::vector<std::vector<Point>>& chains, const std::vector<std::vector<uint8_t>>& fixed, std::vector<std::vector<QuadBezier>>& results, float eps ) { float retract_eps = std::min(eps, 0.5f); for (size_t i = 0; i < chains.size(); ++i) { const std::vector<Point>& chain = chains[i]; const int n = static_cast<int>(chain.size()); std::vector<QuadBezier> result; if (n < 2) { results.push_back(result); continue; } + + // Safely access fixed mask for this chain + const std::vector<uint8_t>& fixed_mask = + (i < fixed.size()) ? fixed[i] : std::vector<uint8_t>{};Then use
fixed_maskinstead offixed[i]at line 134:for (int k = 1; k < n - 1; ++k) - if (k < static_cast<int>(fixed[i].size()) && fixed[i][k]) + if (k < static_cast<int>(fixed_mask.size()) && fixed_mask[k]) bounds.push_back(k);🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 104-104: The function 'dp_curve_reduction' is never used.
(unusedFunction)
🤖 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 `@core/src/internal/douglas_peucker.cpp` around lines 104 - 117, In the dp_curve_reduction function, add a defensive bounds check before accessing fixed[i] in the loop that iterates over chains. Before accessing the fixed vector at index i, verify that i is within the valid range of fixed.size(). If the index is out of bounds, either skip processing that chain or use a default value for the fixed mask. This ensures the function gracefully handles cases where the caller passes mismatched vector sizes rather than causing undefined behavior from out-of-bounds access.core/src/internal/image_utils.cpp (1)
120-123:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate the upper bound of
num_thresholdsbefore computingREGION_SIZE.Line 123 can still evaluate to zero when
num_thresholds > 255, leaving a reachable undefined-behavior path in quantization math. Guard/clamp the full valid range first.Proposed fix
void threshold_image(uint8_t* ptr, const int width, const int height, const int num_thresholds) { - if (num_thresholds <= 0) { + if (num_thresholds <= 0 || num_thresholds > 255) { return; } - const uint8_t REGION_SIZE(255 / num_thresholds); // Size of buckets per colour + const uint8_t REGION_SIZE {static_cast<uint8_t>(255 / num_thresholds)}; // Size of buckets per colour🤖 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 `@core/src/internal/image_utils.cpp` around lines 120 - 123, The validation check for num_thresholds only guards against values less than or equal to zero, but does not validate the upper bound. When num_thresholds exceeds 255, the integer division in the REGION_SIZE calculation (255 / num_thresholds) evaluates to zero, creating undefined behavior in the quantization logic. Add an additional validation check after the existing num_thresholds <= 0 guard to ensure num_thresholds does not exceed 255, either by clamping the value to a valid range or returning early if the upper bound is violated, before the REGION_SIZE constant is computed.Dockerfile.dev (1)
146-149:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
find . -name "Dockerfile.dev" -type fRepository: Ryan-Millard/Img2Num
Length of output: 81
🏁 Script executed:
wc -l ./Dockerfile.devRepository: Ryan-Millard/Img2Num
Length of output: 85
🏁 Script executed:
sed -n '140,155p' ./Dockerfile.devRepository: Ryan-Millard/Img2Num
Length of output: 739
🏁 Script executed:
cat -n ./Dockerfile.dev | head -20Repository: Ryan-Millard/Img2Num
Length of output: 1166
🏁 Script executed:
web_search What is the official checksum or SHA256 for the just.systems installer script?Repository: Ryan-Millard/Img2Num
Length of output: 158
🌐 Web query:
just systems installer script SHA256 checksum verification💡 Result:
The official
justcommand runner documentation provides a direct installation method via a shell script but does not incorporate native SHA256 checksum verification within theinstall.shscript itself [1][2]. To verify the integrity of thejustinstallation, you should use the pre-built binary archives available on the official releases page instead of piping the install script directly tobash[1][2]. ### Steps for Verified Installation 1. Navigate to the officialjustreleases page on GitHub [1][2]. 2. Download the desired pre-built binary archive for your platform [1]. 3. Download the correspondingSHA256SUMSfile provided with the release [1]. 4. Verify the integrity of the archive by running the following command in the same directory [1]:bash shasum --algorithm 256 --ignore-missing --check SHA256SUMSIf you prefer using the installation script while ensuring security, community tools likechecksum.shcan be used to manually verify the script's hash before execution, though this is not an official feature ofjust[3][4]. Note that the official documentation warns that theinstall.shscript may be subject to rate-limiting on shared IP addresses (such as GitHub Actions) and recommends using specific version tags or aGITHUB_TOKENto improve reliability [1][2].Citations:
- 1: https://just.systems/man/en/pre-built-binaries.html
- 2: https://github.com/casey/just?tab=readme-ov-file
- 3: https://checksum.sh/
- 4: https://news.ycombinator.com/item?id=33375554
Pin and verify the
justinstaller to ensure supply-chain integrity.The build downloads an unverified remote installer script without checksum validation, weakening reproducibility and supply-chain integrity.
The official
justdocumentation recommends using pre-built binaries with official SHA256SUMS verification rather than piping the installer script. Alternatively, if using the installer script, add SHA256 checksum verification:🔒 Suggested hardening
+# Pin installer metadata (obtain SHA256 from https://github.com/casey/just/releases) +ARG JUST_INSTALLER_URL=https://just.systems/install.sh +ARG JUST_INSTALLER_SHA256=<expected_sha256> + # Just package manager -RUN curl --proto '=https' --tlsv1.2 -sSf https://just.systems/install.sh -o setup_just.sh && \ - bash setup_just.sh --to /usr/local/bin && \ +RUN curl --proto '=https' --tlsv1.2 -fsSL "$JUST_INSTALLER_URL" -o setup_just.sh && \ + echo "${JUST_INSTALLER_SHA256} setup_just.sh" | sha256sum -c - && \ + bash setup_just.sh --to /usr/local/bin && \ rm setup_just.sh🤖 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 `@Dockerfile.dev` around lines 146 - 149, The `just` package manager installer script is being downloaded and executed without any checksum verification, which poses a supply-chain security risk. Replace the current approach in the RUN instruction that downloads the unverified installer script from https://just.systems/install.sh with either: (1) using official pre-built binaries for the target architecture with SHA256SUMS verification from the official `just` repository, or (2) if continuing with the installer script, download both the installer script and the corresponding SHA256SUMS file, verify the script's checksum matches the official one before execution, and then proceed with running the verified script. This will ensure reproducibility and supply-chain integrity by validating the integrity of downloaded artifacts.Source: Coding guidelines
example-apps/console-cpp/main.cpp (1)
68-68:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse or remove
res_svgto avoid strict-build failures.Line 68 creates
res_svgbut never consumes it. With warning-as-error settings, this can fail the example target build.Proposed fix
std::string res_svg {img2num::labels_to_svg(img_data, out_labels, width, height, 100, 10)}; @@ std::string res_svg2 {img2num::image_to_svg(img_data, width, height, config)}; + if (res_svg != res_svg2) { + std::cerr << "Warning: labels_to_svg and image_to_svg outputs differ.\n"; + }🤖 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 `@example-apps/console-cpp/main.cpp` at line 68, The variable res_svg is created by calling img2num::labels_to_svg but is never used afterwards, which causes a build failure with strict compiler settings. Either remove the res_svg variable declaration entirely if the generated SVG is not needed, or use the variable by writing it to output, saving it to a file, printing it, or passing it to another function that consumes it.Justfile (1)
52-65:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast on unsupported
build/cleantargets.At Line 53 and Line 62, unknown targets currently fall through and return success. That can hide typos and break scripts silently.
✅ Suggested fix
build target: case "{{ target }}" in \ cpp) just build-c-cpp ;; \ js) just build-wasm ;; \ py) just build-py ;; \ all) just build-c-cpp build-wasm build-py react-js build docs build ;; \ + *) echo "Unknown build target: {{ target }}" >&2; exit 1 ;; \ esac clean target: `@echo` "Remove {{ target }} folders" case "{{ target }}" in \ cpp) rm -rf build-c-cpp/ ;; \ js) rm -rf build-wasm/ ;; \ + *) echo "Unknown clean target: {{ target }}" >&2; exit 1 ;; \ esac🤖 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 `@Justfile` around lines 52 - 65, The build and clean recipes have case statements that silently succeed when an unrecognized target is provided, which can hide typos and cause scripts to fail unexpectedly. Add a default case clause to both the build target and clean target recipes using the pattern `*) echo "Error: unsupported target" >&2; exit 1 ;;` at the end of each case statement to explicitly fail when an invalid target is passed, ensuring unknown targets are caught immediately.packages/py/img2num/api.py (1)
64-65:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a backward-compatible default for
min_thicknessin the public Python API.Line 64 makes
min_thicknessrequired, which is a breaking API change for existing callers oflabels_to_svg. Give it a default (typically0to preserve prior behavior) and keep forwarding it downstream.Proposed fix
-def labels_to_svg(data: npt.NDArray[np.uint8], labels: npt.NDArray[int], min_area: int, min_thickness: int, *, width: int, height: int) -> str: +def labels_to_svg( + data: npt.NDArray[np.uint8], + labels: npt.NDArray[int], + min_area: int, + min_thickness: int = 0, + *, + width: int, + height: int, +) -> str: return _labels_to_svg(data, labels, width, height, min_area, min_thickness)🤖 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 `@packages/py/img2num/api.py` around lines 64 - 65, The `labels_to_svg` function currently has `min_thickness` as a required parameter, which breaks backward compatibility for existing callers. Add a default value of `0` to the `min_thickness` parameter in the function signature to restore backward compatibility while continuing to forward it to the downstream `_labels_to_svg` call.
4b79b62 to
2f9d028
Compare
|
incredibly annoying the cibuildwheel spins up it's own docker container and then has to recompile Dawn each time |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/release.yml:
- Around line 103-111: The GitHub Actions workflow in the release.yml file lacks
concurrency controls which can cause redundant builds when multiple commits or
PRs are pushed rapidly. Add a concurrency block at the workflow level
(positioned after the name field and before the on field) that groups runs by
workflow and the current git reference, and cancels in-progress runs for
non-main branches to optimize build resources and prevent unnecessary duplicate
builds while preserving all runs on the main branch.
In `@bindings/py/CMakeLists.txt`:
- Line 13: The find_package(Python3) call in bindings/py/CMakeLists.txt uses the
Development.Module component which requires CMake 3.18 or later, but the
project's cmake_minimum_required in the root CMakeLists.txt is set to version
3.16, creating a compatibility issue. Either update cmake_minimum_required to
3.18 in the root CMakeLists.txt to match the component requirement, or replace
the Development.Module component with just Development in the find_package call
to maintain compatibility with CMake 3.16 and 3.17. Choose the option that best
aligns with the project's supported CMake versions.
In `@CMakeLists.txt`:
- Around line 30-65: The code uses block() and endblock() which are not
available in CMake 3.16 (the project's minimum required version), causing
configuration to fail on CMake 3.16 through 3.24. Remove the block() statement
at the beginning and the endblock() statement at the end of the
IMG2NUM_DEBUG_CACHE_VARIABLES_DUMP debug dump section, keeping the if condition
and all the debug output logic intact, since this debug dump does not require a
separate scope.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fda21127-03de-4774-b8a8-9206a3e50764
📒 Files selected for processing (5)
.github/workflows/release.ymlCMakeLists.txtbindings/py/CMakeLists.txtbindings/py/src/img2num_pybind.cpppyproject.toml
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: build-c-cpp-native (ubuntu-latest, c)
- GitHub Check: build-c-cpp-native (windows-latest, c)
- GitHub Check: build-c-cpp-native (windows-latest, cpp)
- GitHub Check: build-py (macos-latest)
- GitHub Check: build-c-cpp-native (macos-latest, c)
- GitHub Check: build-c-cpp-native (macos-latest, cpp)
- GitHub Check: build-c-cpp-native (ubuntu-latest, cpp)
- GitHub Check: build-py (windows-latest)
- GitHub Check: build-py (ubuntu-latest)
- GitHub Check: Build C/C++ / Build Python
- GitHub Check: Build C/C++ / Build WASM (bindings/js)
- GitHub Check: Build C/C++ / Build C & C++
- GitHub Check: Build Documentation Site / Build Docusaurus Site
- GitHub Check: Lint & Validate Code
- GitHub Check: Analyze (c-cpp)
🧰 Additional context used
📓 Path-based instructions (8)
**
⚙️ CodeRabbit configuration file
**: # Contributing to Img2NumWant to contribute to Img2Num? There are a few things you need to know.
We wrote a contribution guide to help you get started.
A few important points:
- Add tests with your PR — new features and bug fixes must include tests where appropriate. PRs without tests are unlikely to be approved.
- Follow the repository's coding style rules.
- Use the issue and PR templates when filing issues or submitting code. Your PR will be rejected if you don't.
If you're unsure what to change, open a discussion and someone will assist you.
Questions?
If you have questions or need help:
- Open a discussion
- Create an issue
- Check existing PRs for ideas
Thank you for improving Img2Num! 🎨🚀
**: BasedOnStyle: LLVM
Standard: c++20--- Basic formatting ---
IndentWidth: 4
ColumnLimit: 100
TabWidth: 4--- Braces ---
Cpp11BracedListStyle: true
SpaceBeforeCpp11BracedList: true
BreakBeforeBraces: Attach--- Braced initializers ---
Cpp11BracedListStyle: true
--- Constructor initializer lists ---
PackConstructorInitializers: Never
BreakConstructorInitializers: BeforeComma
ConstructorInitializerIndentWidth: 4--- Alignment ---
AlignAfterOpenBracket: BlockIndent
--- Pointers ---
PointerAlignment: Left
--- Includes ---
IncludeBlocks: Regroup
SortIncludes: CaseInsensitive--- Extern "C" cleanliness ---
IndentExternBlock: NoIndent
--- Lambdas ---
AllowShortLambdasOnASingleLine: Inline
--- Functions ---
AllowShortFunctionsOnASingleLine: None
**: root = true-------------------------
Global defaults
-------------------------
[*]
charset = utf-8
end_of_line = lf
indent_style = space
indent_size = 2
trim_trailing_whitespace = true
insert_final_newline = true
max_line_lengt...
Files:
pyproject.tomlbindings/py/CMakeLists.txtbindings/py/src/img2num_pybind.cppCMakeLists.txt
**/*.txt
📄 CodeRabbit inference engine (.editorconfig)
**/*.txt: Do not enforce maximum line length for text files
Use 2-space indentation for text files
Files:
bindings/py/CMakeLists.txtCMakeLists.txt
bindings/**
⚙️ CodeRabbit configuration file
bindings/**: Language bindings for the Img2Num library. Ensure the public API surface matches
the core C/C++ headers and that error propagation is handled correctly for each
binding language. Also ensure the relevant docstrings are present to enable
auto-generation of documentation.
Files:
bindings/py/CMakeLists.txtbindings/py/src/img2num_pybind.cpp
**/*.{hpp,cpp,c,h}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{hpp,cpp,c,h}: Use 4-space indentation for C/C++ files
Maintain 120 character maximum line length for C/C++ files
Files:
bindings/py/src/img2num_pybind.cpp
**/*.{cpp,cc,cxx,c++,h,hpp,hxx,h++}
📄 CodeRabbit inference engine (.clang-format)
**/*.{cpp,cc,cxx,c++,h,hpp,hxx,h++}: Use LLVM coding style as the base style for C++ code
Use C++20 standard for all C++ code
Use an indent width of 4 spaces for C++ code
Limit line length to 100 columns in C++ code
Use 4 spaces for tab width in C++ code
Use C++11 braced list style with space before braced lists in C++ code
Attach opening braces to the same line (BreakBeforeBraces: Attach) in C++ code
Never pack constructor initializer lists on a single line in C++ code
Break constructor initializer lists before comma in C++ code
Use 4 spaces for constructor initializer list indentation in C++ code
Use block indent alignment after opening brackets in C++ code
Align pointers to the left in C++ code
Regroup and organize include blocks in C++ code
Sort includes case-insensitively in C++ code
Do not indent extern "C" blocks in C++ code
Allow short lambdas only inline on a single line in C++ code
Do not allow short functions on a single line in C++ code
Files:
bindings/py/src/img2num_pybind.cpp
**/*.{cpp,c,h,hpp}
⚙️ CodeRabbit configuration file
**/*.{cpp,c,h,hpp}: - For any C/C++ code outside core/ (e.g. bindings, example-apps), apply the same
memory-safety and style standards as the core. Formatting via./img2num format-cpp.
- Doxygen docstrings are required.
Files:
bindings/py/src/img2num_pybind.cpp
{CMakeLists.txt,**/*.cmake}
📄 CodeRabbit inference engine (.editorconfig)
{CMakeLists.txt,**/*.cmake}: Use 2-space indentation for CMake configuration files
Maintain 120 character maximum line length for CMake files
Files:
CMakeLists.txt
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: GitHub Actions workflows. Review for:
- SHA-pinned action versions for third-party actions (security best practice).
- Secrets accessed only via ${{ secrets.* }} — never hardcoded.
- Least-privilege permissions on each job/workflow.
- Correct job dependency ordering (needs:) and if/condition logic.
Files:
.github/workflows/release.yml
🧠 Learnings (7)
📚 Learning: 2025-12-31T17:46:54.476Z
Learnt from: fransafu
Repo: Ryan-Millard/Img2Num PR: 176
File: src/wasm/modules/image/src/bilateral_filter.cpp:98-101
Timestamp: 2025-12-31T17:46:54.476Z
Learning: In bilateral_filter.cpp (src/wasm/modules/image/src/bilateral_filter.cpp), rely on the fact that the center pixel contributes a weight of exactly 1.0 to both spatial and range components (exp(0) = 1) so the normalization sum cannot be zero. This implies you do not need explicit guards against division by zero for normalization in this implementation.
Applied to files:
bindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-01-06T04:56:57.269Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/kmeans.cpp:128-134
Timestamp: 2026-01-06T04:56:57.269Z
Learning: In C++ sources compiled for WASM with Emscripten/Clang, designated initializers (e.g., RGBXY{.r = ..., .g = ...}) are allowed as a C++17 extension. When reviewing code that relies on designated initializers, verify that the target toolchain enables CXX_STANDARD 17 or higher and that the build system (CMake/emsdk) uses Emscripten with a compatible clang. If not, avoid such initializers or provide portable alternatives.
Applied to files:
bindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-01-06T21:06:24.476Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/cielab.cpp:43-57
Timestamp: 2026-01-06T21:06:24.476Z
Learning: In the Img2Num project, prefer recommending and using the provided docker/script wrappers (e.g., ./img2num format-wasm, ./img2num clean-wasm) instead of invoking local tools directly (e.g., clang-format -i). This reduces dependency requirements for users and ensures consistent tooling across environments. Apply this guidance to C++ source files under the project when reviewing changes.
Applied to files:
bindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-01-19T00:02:34.957Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 234
File: src/wasm/modules/image/src/kmeans.cpp:79-81
Timestamp: 2026-01-19T00:02:34.957Z
Learning: In C++ multithreading contexts, prefer unsigned types for thread-count-like parameters (e.g., n_threads) to prevent negative values. Validate that the value is at least 1 before any division to avoid divide-by-zero at runtime. At the start of functions handling such values, create a safe count like: const unsigned int thread_count{std::max(1u, n_threads)}; This ensures non-negative, non-zero usage for divisions and related arithmetic. Apply this pattern to similar parameters across C++ modules, not just the specific file.
Applied to files:
bindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-02-25T21:24:19.036Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 272
File: core/src/internal/bilateral_filter_gpu.cpp:171-185
Timestamp: 2026-02-25T21:24:19.036Z
Learning: Adopt brace initialization (e.g., Type var{}) over copy initialization (e.g., Type var = {}) for zero-initialization in C++ across the repository. This should apply to variables, arrays, and structs in most C++ files (e.g., core/src/internal/bilateral_filter_gpu.cpp). It improves safety by avoiding narrowing conversions and makes initialization intent explicit.
Applied to files:
bindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-05-01T22:50:11.527Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 339
File: release-please-config.json:18-47
Timestamp: 2026-05-01T22:50:11.527Z
Learning: In this repo, release-please-action v4 preserves '/' verbatim in slash-containing path-based package keys when emitting GitHub Actions output names (e.g., `bindings/c--release_created`). When referencing these step outputs in `job.outputs` (and other expressions), use bracket notation with the exact output name: `${{ steps.release.outputs['bindings/c--release_created'] }}` rather than dot notation. If needed, map the complex step output to a clean job-level output alias so downstream jobs can use dot notation via that alias.
Applied to files:
.github/workflows/release.yml
📚 Learning: 2026-05-19T17:30:09.565Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 375
File: .github/workflows/cmake-build.yml:86-88
Timestamp: 2026-05-19T17:30:09.565Z
Learning: In Ryan-Millard/Img2Num CI/workflow YAMLs, any `uv sync` command used for the Python package build must include `--no-build-isolation` (do not remove it). If you need deterministic dependency installs for CI, you may add `--frozen` alongside it (e.g., `uv sync --frozen --no-build-isolation`), and it should not conflict with the repo’s build setup.
Applied to files:
.github/workflows/release.yml
🪛 zizmor (1.26.1)
.github/workflows/release.yml
[warning] 3-7: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
[info] 84-84: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 85-85: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 86-86: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 89-89: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 90-90: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 107-107: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 108-108: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🔇 Additional comments (10)
CMakeLists.txt (3)
19-28: LGTM!
70-97: LGTM!
101-106: LGTM!pyproject.toml (2)
42-42: LGTM!
90-91: LGTM!.github/workflows/release.yml (2)
6-7: LGTM!
83-90: LGTM!bindings/py/src/img2num_pybind.cpp (3)
101-101: LGTM!
124-137: LGTM!
186-187: LGTM!Also applies to: 198-198, 208-208
It is quite annoying. I don't know what to do about it. |
There should be a way to copy artifacts out of the cibuildwheel docker and then upload them. So for example, in the first ubuntu cibuildwheel run it'll compile Dawn, we can copy out the compiled libs, and then push them into the next cibuildwheel run. Same for the windows version. would need to experiment to figure out exactly how to pull this off |
If you'd like to work on it and figure out how to do it, that's fine. I personally don't think that it's very necessary right now - it could probably come in a follow-up PR because it at least works right now. I only have it running on this PR. It won't run on any other ones because it takes so long. It'll dry-run on |
Oh ok if it's just something that runs in-frequently then the long runs are fine |
It's only for releases and validation. The builds in Nowadays, we push to |
|
@coderabbitai please generate a good commit message and description for when I squash this. |
|
Tip For best results, initiate chat on the files or code changes.
Subject line: Body: |
2 participants
What was changed & why
Fixes: #432
Fix build failures in .github/workflows/release.yml by expanding the
CMake Configure step to pass the full set of required options, including
an IMG2NUM_BUILD_C toggle derived from component metadata, and disabling
Python/examples builds during the native C/C++ build phase.
Standardise all project-level CMake cache variables under the IMG2NUM_
namespace (IMG2NUM_BUILD_C, IMG2NUM_BUILD_PYTHON, IMG2NUM_BUILD_EXAMPLES,
IMG2NUM_DEBUG_CACHE_VARIABLES_DUMP) and make subdirectory inclusion
conditional on these flags. Add an optional ANSI-coloured cache-variable
debug dump to aid configuration inspection.
Update pyproject.toml to use the renamed -DIMG2NUM_BUILD_PYTHON=ON flag
and add a [tool.cibuildwheel] section with build-verbosity = 1.
Fix the Python bindings CMake to require Development.Module instead of
Development when locating Python3, resolving scikit-build-core
compatibility issues.
The release workflow dry-runs on dev and publishes to PyPI only when
triggered by release-please on main. Caching of Dawn build artifacts
across cibuildwheel runs can be explored in a follow-up PR.