diff --git a/AGENTS.md b/AGENTS.md index acf54fb7bcc..ea414b27efa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -6,18 +6,24 @@ Apache 2.0 licensed. Build tool: **Pixi** (wraps CMake + Ninja). ## Context to Load on Demand -Load only what your task requires: +Load only what your task requires. Files are small and focused — load +the minimum set for the task at hand. | Task | Read | |------|------| -| Understanding the codebase layout | [./Documentation/AI/architecture.md](./Documentation/AI/architecture.md) | -| Building or configuring ITK | [./Documentation/AI/building.md](./Documentation/AI/building.md) | -| Writing or running tests | [./Documentation/AI/testing.md](./Documentation/AI/testing.md) | -| Code style, naming conventions | [./Documentation/AI/style.md](./Documentation/AI/style.md) | -| Writing ITK C++ classes, CMake build configuration, and Python wrapping configuration | [./Documentation/AI/conventions.md](./Documentation/AI/conventions.md) | -| Creating a git commit with C++ changes | [./Documentation/AI/git-commits.md](./Documentation/AI/git-commits.md), [./Documentation/AI/compiler-cautions.md](./Documentation/AI/compiler-cautions.md), [./Documentation/AI/building.md](./Documentation/AI/building.md), [./Documentation/AI/testing.md](./Documentation/AI/testing.md) | -| Opening or updating a pull request | [./Documentation/AI/pull-requests.md](./Documentation/AI/pull-requests.md) | -| Avoiding compiler-specific pitfalls and refactoring hazards | [./Documentation/AI/compiler-cautions.md](./Documentation/AI/compiler-cautions.md) | +| Understanding the codebase layout | [architecture.md](./Documentation/AI/architecture.md) | +| Building or configuring ITK | [building.md](./Documentation/AI/building.md) | +| Writing or running tests | [testing.md](./Documentation/AI/testing.md) | +| Code style, formatting, naming | [enforced-code-style.md](./Documentation/AI/enforced-code-style.md) | +| Writing or reviewing C++ code | [code-review-lessons.md](./Documentation/AI/code-review-lessons.md) | +| Writing ITK C++ classes, CMake, Python wrapping | [conventions.md](./Documentation/AI/conventions.md) | +| Avoiding compiler pitfalls and refactoring hazards | [compiler-cautions.md](./Documentation/AI/compiler-cautions.md) | +| Creating a **DOC:** commit | [git-commits.md](./Documentation/AI/git-commits.md) | +| Creating a **STYLE:** commit | [git-commits.md](./Documentation/AI/git-commits.md), [enforced-code-style.md](./Documentation/AI/enforced-code-style.md) | +| Creating a **BUG:** or **ENH:** commit | [git-commits.md](./Documentation/AI/git-commits.md), [compiler-cautions.md](./Documentation/AI/compiler-cautions.md), [testing.md](./Documentation/AI/testing.md) | +| Creating a **COMP:** commit | [git-commits.md](./Documentation/AI/git-commits.md), [compiler-cautions.md](./Documentation/AI/compiler-cautions.md) | +| Commit or PR attribution | [attribution.md](./Documentation/AI/attribution.md) | +| Opening or updating a pull request | [pull-requests.md](./Documentation/AI/pull-requests.md), [attribution.md](./Documentation/AI/attribution.md) | ## Critical Pitfalls diff --git a/Documentation/AI/README.md b/Documentation/AI/README.md index 30e698fa257..dfb0fe45269 100644 --- a/Documentation/AI/README.md +++ b/Documentation/AI/README.md @@ -24,6 +24,7 @@ Add file to table in AGENTS.md for routing: | File | When to load | |------|-------------| | `building.md` | Configuring or building ITK with Pixi or CMake | +| `code-review-lessons.md` | Writing or reviewing C++ code — recurring reviewer concerns | | `testing.md` | Writing, running, or converting tests | ## Adding a New Context File diff --git a/Documentation/AI/attribution.md b/Documentation/AI/attribution.md new file mode 100644 index 00000000000..50368413a87 --- /dev/null +++ b/Documentation/AI/attribution.md @@ -0,0 +1,122 @@ +# Commit and PR Attribution Rules + +## Commits: what and why only + +Commit messages describe **what** changed and **why**. They do not +describe how the change was produced. AI tool details, model names, and +tool-specific trailers (`Tool-Assisted:`, `Assisted-by:`) do **not** +belong in commit messages. + +Keep commit messages concise — a 1-line subject (≤ 78 chars) plus a +short body describing intent. Long rationale goes in the PR description, +not the commit body. + +## Co-Authored-By + +`Co-Authored-By:` is for contributors whose intellectual contribution +materially shaped the commit — typically a co-developer or a reviewer +whose feedback drove a design change. + +**For AI tools: never.** The human who prompted, reviewed, and +committed the code bears authorship responsibility. `Co-Authored-By:` +implies accountability — AI tools cannot be paged when the code breaks, +cannot defend design decisions in review, and cannot maintain the code +going forward. AI assistance is disclosed in the PR description instead +(see below). + +**For reviewers:** use `Co-Authored-By:` only when a reviewer's +feedback **materially shaped the design** of the commit (e.g., "keep +v142 but migrate to windows-2022" led to a different implementation +approach). A reviewer who caught a typo or requested a rename is a +reviewer, not a co-author. For review acknowledgment without +co-authorship, mention the reviewer in prose in the commit body or PR +description: + +``` +Addresses dzenanz's review: restored v142 toolset entry with updated +generator and runner image. +``` + +**The responsibility test:** if this code has a bug, who gets paged? +If the answer is a human, that human may be a Co-Author. If the answer +is "whoever committed it," the committer is the sole author. + +## AI disclosure: PR description only + +The PR description is the **sole location** for AI tool disclosure. +Commits are clean. + +When AI tools made a substantive contribution (root-cause analysis, +algorithm design, hypothesis testing), disclose in the PR body: + +```markdown +Short visible summary of what changed and why. + +
+AI assistance + +- Tool: Claude Code (claude-opus-4-6) +- Role: root-cause analysis of ccache hit rate regression +- Contribution: identified CCACHE_NODIRECT=1 as sole cause by + comparing ARM CI with Azure DevOps pipeline configurations +- All code was reviewed and tested locally before committing + +
+``` + +**No disclosure needed** for mechanical changes: reformatting, rename +refactoring, boilerplate generation, applying a well-known pattern the +human specified. + +## PR body format: concise by default, details on request + +PR descriptions must be **short and reviewer-friendly by default**. +A reviewer should understand the PR's purpose in under 10 seconds +from the visible text. + +- **Lead with a 1-3 line summary** — what changed and why. +- **Sequester longer analysis inside `
` blocks** — root cause + analysis, AI assistance details, test output, failed approaches, + status tables, and dependency discussion. +- **Keep visible text to actionable items** — if the reviewer must read + it to know what to do, keep it visible. If it's background context + for those who want the deep dive, collapse it. + +```markdown +Fix float-precision division in BresenhamLine::BuildLine. Closes #6054. + +
+Root cause + +The integer division `abs(dx)/abs(dy)` truncated to zero for +shallow angles, producing incorrect line segments... + +
+ +
+AI assistance + +Claude Code identified the truncation pattern and suggested the +`static_cast` fix. Verified locally with the GTest suite. + +
+``` + +## External context + +When Discourse threads, issues, or other sources informed a commit, +attribute in prose with stable links. Replace transient URLs (CI build +links, Azure DevOps runs, CDash build URLs) with extracted context: + +``` +# BAD — transient links that will expire: +See https://dev.azure.com/itkrobot/.../buildId=14265 + +# GOOD — context preserved: +Based on discussion in https://discourse.itk.org/t/7745 (patches +for 5.4.6). Azure DevOps ITK.macOS.Python failed with exit code +255 (dashboard script treats warnings as fatal; CDash shows +0 compile errors and 0 test failures). +``` + +[attribution-issue]: https://github.com/InsightSoftwareConsortium/ITK/issues/6055 diff --git a/Documentation/AI/code-review-lessons.md b/Documentation/AI/code-review-lessons.md new file mode 100644 index 00000000000..6288e10510f --- /dev/null +++ b/Documentation/AI/code-review-lessons.md @@ -0,0 +1,310 @@ +# ITK Code Review Lessons — Recurring Reviewer Concerns + +Distilled from 8,484 inline review comments across 1,457 pull requests +spanning 2017–2026. These are patterns that ITK's core reviewers flag +repeatedly; an AI assistant that avoids them will produce PRs that pass +review with fewer round-trips. + +--- + +## 1. Remove All Orphaned Artifacts After Refactoring + +**Flagged on 31% of reviewed PRs (453/1,457). Active 2018–2026.** + +When you remove the last user of a helper function, a local variable, +an `#include`, or a type alias — also remove the definition itself. +Reviewers catch orphaned code more than any other single issue. + +```cpp +// BAD — removed the std::cout print that used chBuffer, +// but left the clGetPlatformInfo() call that populated it. +clGetPlatformInfo(platform, CL_PLATFORM_NAME, sizeof(chBuffer), chBuffer, nullptr); + +// GOOD — removed both the consumer AND the producer. +``` + +**Checklist before submitting a refactoring PR:** +- Grep for every symbol you modified or deleted. +- If a helper, typedef, or include has zero remaining callers, remove it. +- If a `using` alias was only public for one consumer, move it to `private` + or remove it entirely. + +--- + +## 2. Test Quality and GTest Conventions + +**Flagged on 28% of reviewed PRs (411/1,457). Active 2017–2026.** + +### Unique suite names per `.cxx` file + +```cpp +// BAD — "HeavisideStepFunction" reused across multiple test files: +TEST(HeavisideStepFunction, ConvertedLegacyTest) // in file A +TEST(HeavisideStepFunction, AnotherTest) // in file B + +// GOOD — one unique suite name per .cxx file: +TEST(SinRegularizedHeavisideStepFunction, ConvertedLegacyTest) +``` + +### Use `ConvertedLegacyTest` for migrated CTest tests + +When converting a legacy `itkFooTest.cxx` to GTest, name the test +`TEST(Foo, ConvertedLegacyTest)` unless the test has a more specific +purpose worth naming. + +### Non-fatal assertions need null guards + +`ITK_TEST_EXPECT_TRUE` is non-fatal — it records failure but continues. +If a `dynamic_cast` might return null, guard before dereferencing: + +```cpp +// BAD — continues past null and crashes: +auto * p = dynamic_cast(base.GetPointer()); +ITK_TEST_EXPECT_TRUE(p != nullptr); +p->DoSomething(); // CRASH if dynamic_cast failed + +// GOOD — bail immediately on null: +auto * p = dynamic_cast(base.GetPointer()); +if (p == nullptr) +{ + std::cerr << "dynamic_cast failed" << std::endl; + return EXIT_FAILURE; +} +``` + +--- + +## 3. Include and Header Hygiene + +**Flagged on 21% of reviewed PRs (313/1,457). Active 2018–2026.** + +- Include only what you use. Do not leave includes for removed code. +- Prefer forward declarations in headers when only a pointer or + reference is needed. +- After removing code, check whether any `#include` directives became + orphaned. + +--- + +## 4. Naming Clarity + +**Flagged on 17% of reviewed PRs (254/1,457). Active 2018–2026.** + +- Variable names should describe what they hold, not how they were computed. +- After applying a limit or filter, rename the variable to reflect its + new meaning (e.g., `nodes` → `displayed_nodes` after truncation). +- Magic numbers should be named constants or use ITK's existing named + constants (e.g., `itk::Statistics::MersenneTwisterRandomVariateGenerator::DefaultSeed` + instead of `121212`). + +--- + +## 5. Style Consistency Within a Function + +**Flagged on 15% of reviewed PRs (214/1,457). Active 2018–2026.** + +After a partial fix, check that the modified code is consistent with the +rest of the function and file: + +- If a function parameter type was updated, check that the function body + uses the same qualified form. +- If a naming pattern was corrected, apply the correction to all similar + instances in the same scope — do not leave a mix. +- Sub-section numbering, comment formatting, and JSON keys should be + consistent within and across files. + +--- + +## 6. Error Handling and Exception Safety + +**Flagged on 14% of reviewed PRs (201/1,457). Active 2018–2026.** + +- Check return values from functions that can fail (`dynamic_cast`, + Python C API calls, file I/O). A `nullptr` or error return passed + silently to the next line is a crash. +- After removing cleanup code (e.g., `delete m_Writer; m_Writer = nullptr;`), + verify the replacement (`unique_ptr`, RAII) provides equivalent + exception-safety guarantees. +- `EXPECT_NO_THROW` in GTest should wrap only the call being tested, + not surrounding side-effects like `std::cout`. + +--- + +## 7. Signed/Unsigned Conversions and `size_t` + +**Flagged on 8% of reviewed PRs (120/1,457). Active 2018–2026.** + +- Avoid narrowing conversions between signed and unsigned types. + Prefer ITK's `SizeValueType` or `unsigned int` consistently. +- Do not add `static_cast` just to silence a warning — ask whether the + conversion is actually safe. Unnecessary casts obscure real bugs. +- For template parameters, prefer `unsigned{VRows}` over + `static_cast(VRows)` — it is type-safe and rejects + narrowing. + +--- + +## 8. Locale-Safe Numeric Parsing + +**Flagged on 8% of reviewed PRs (113/1,457). Active 2018–2026.** + +`std::stod`, `std::stof`, `atof`, and `std::to_string` are +locale-dependent. Under European locales they produce/consume `,` +instead of `.` as the decimal separator, silently corrupting +medical image metadata. + +```cpp +// BAD — locale-dependent: +double value = std::stod(str); +buffer << std::fixed << value; + +// GOOD — locale-safe: +buffer << itk::ConvertNumberToString(value); +``` + +Use `itk::ConvertNumberToString()` for serialization. +See `itk-locale-safe-migration` for the full set of affected functions. + +--- + +## 9. When to Use `auto` + +**Flagged on 7% of reviewed PRs (105/1,457). Active 2018–2026.** + +ITK is not anti-`auto`, but reviewers reject it when the deduced type is +not obvious from the initializer. + +```cpp +// GOOD — type is obvious from the RHS: +const auto size = image->GetLargestPossibleRegion().GetSize(); +auto filter = MedianImageFilter::New(); +const auto it = container.begin(); + +// BAD — reader cannot guess the deduced type: +const auto value = interp->EvaluateDerivativeAtContinuousIndex(index); +// (value is a CovariantVector — not obvious) + +// BETTER — spell out non-obvious types: +const CovariantVectorType value = interp->EvaluateDerivativeAtContinuousIndex(index); +``` + +**Rule of thumb:** use `auto` when the type name appears on the same line +(factory methods, iterators, casts) or is unambiguous from the method name +(`GetSize()`, `GetSpacing()`). Spell out the type when the return type +requires knowledge of the class's internal typedefs. + +--- + +## 10. ITK Single-Expression Initialization + +**Flagged on 4% of reviewed PRs (57/1,457). Active 2018–2026.** + +Prefer single-expression initialization over declare-then-mutate +patterns. This applies to `Size`, `Index`, `Offset`, and similar +aggregate types that provide `Filled()`, as well as `Image::Allocate`: + +```cpp +// BAD — two-line declare-then-fill: +SizeType size; +size.Fill(2); + +// GOOD — single expression: +constexpr auto size = SizeType::Filled(2); + +// BAD — zero-initialize in two lines: +IndexType start; +start.Fill(0); + +// GOOD — brace initialization is zero-fill for aggregate types: +constexpr IndexType start{}; + +// BAD — boolean argument is unclear at the call site: +image->Allocate(true); + +// GOOD — named method communicates intent: +image->AllocateInitialized(); +``` + +--- + +## 11. No C-Style Casts; Prefer Local Fixes Over Cascading Changes + +**Flagged on 10 PRs (0.7%). Active 2019–2026. Every instance was a correctness concern.** + +```cpp +// BAD: +unsigned int rows = (unsigned int)VRows; + +// GOOD — prefer no cast; if needed: +unsigned int rows = unsigned{VRows}; // type-safe, rejects narrowing +unsigned int rows = static_cast(VRows); +``` + +Before adding any cast, ask: "Do I actually get a compiler warning without +this?" If not, the cast is unnecessary and obscures the code. + +When a cast exists because a local variable has a mismatched type, it is +often better to change the local variable's type to eliminate the cast +entirely. However, **do not cascade type changes into function signatures, +template parameters, or public API boundaries** to avoid a cast. A small +local `static_cast` or `T{x}` conversion is preferable to changing an +API that downstream consumers depend on. The rule: fix the narrowest +scope that removes the cast without altering any interface. + +--- + +## 12. Redundant Namespace Qualifiers + +**Flagged on 11 PRs (0.8%). Active 2021–2026.** + +Code inside `namespace itk { ... }` should not prefix ITK symbols with +`itk::`. + +```cpp +// BAD — inside a .cxx file that is already in namespace itk: +itk::ConvertNumberToString(value); + +// GOOD: +ConvertNumberToString(value); +``` + +--- + +## 13. AI-Generated Descriptions Must Be Factually Verified + +**Flagged on 1 PR (2026). Severity: high — incorrect claims erode reviewer trust.** + +AI-generated PR descriptions and review summaries have been observed +claiming incorrect counts (e.g., "three temporary variables eliminated" +when only one existed). This has been compared to known LLM counting +errors. + +**Rule:** Before submitting an AI-generated description, manually verify +every concrete claim — counts, variable names, file paths, and behavioral +assertions. If the AI says "N items were changed," count them yourself. + +### Keep commit messages and PR descriptions in sync with scope + +Refactoring, squashing, addressing reviewer comments, and adding fixup +commits frequently change the scope of a PR. After any such change, AI +tools must re-read all commit messages and the PR title/body and verify +they still accurately describe what the PR does. Stale descriptions +that reference removed work, omit added work, or overstate the change +are a common source of reviewer confusion and erode trust in +AI-assisted PRs. + +**Checklist after every scope change:** +- Does the PR title still describe the current change set? +- Does each commit message accurately reflect its diff? +- Were claims about "N files changed" or "M patterns fixed" invalidated + by the scope change? +- If commits were squashed, does the squashed message cover everything + that was folded in? + +--- + +## Methodology + +Generated 2026-04-12 by analyzing 8,484 inline review comments across +1,457 PRs (2017–2026) from the ITK GitHub repository. Topics counted +per distinct PR, not per comment. See the PR description for details. diff --git a/Documentation/AI/compiler-cautions.md b/Documentation/AI/compiler-cautions.md index 4d0d15878e5..cc04548d4aa 100644 --- a/Documentation/AI/compiler-cautions.md +++ b/Documentation/AI/compiler-cautions.md @@ -521,16 +521,89 @@ if (item != nullptr) --- -## 12. clang-tidy Warnings During Refactoring +## 12. KWStyle and Doxygen Pitfalls -### 12a. Do Not Introduce New clang-tidy Diagnostics +### 12a. `enum class` Inside a Class with a `/** */` Comment + +KWStyle's class-comment rule treats `enum class` declarations like a class: +any `/** */` doxygen block immediately preceding an `enum class` must +contain a `\class` tag, otherwise CI fails with: + +``` +error: comment doesn't have \class +``` + +Two acceptable fixes: + +```cpp +// BAD — KWStyle error: +class Foo { + /** Encoding format of the data array found in the file. */ + enum class DataEncoding : std::uint8_t { ASCII, Base64 }; +}; + +// GOOD — plain // comment, not doxygen: +class Foo { + // Encoding format of the data array found in the file. + enum class DataEncoding : std::uint8_t { ASCII, Base64 }; +}; + +// GOOD — doxygen with explicit \class tag: +class Foo { + /** \class DataEncoding + * Encoding format of the data array found in the file. + */ + enum class DataEncoding : std::uint8_t { ASCII, Base64 }; +}; +``` + +**References:** PR #6032 review. + +--- + +## 13. NumericTraits Floating-Point Range + +`NumericTraits::min()` (and `std::numeric_limits::min()`) +returns the smallest **positive** normal value, *not* the most negative +representable value. Computing `max() - min()` for a floating-point pixel +type therefore yields ~`FLT_MAX`, not the dynamic range. Special-case +floating-point types when computing a default dynamic range: + +```cpp +// BAD — yields ~FLT_MAX, not 1.0 or 2.0: +double m_DynamicRange{ NumericTraits::max() - + NumericTraits::min() }; + +// GOOD: +static constexpr double DefaultDynamicRange() +{ + if constexpr (std::is_floating_point_v) + { + return 1.0; // assume normalized + } + return static_cast(NumericTraits::max()) - + static_cast(NumericTraits::min()); +} +``` + +For integer pixel types the formula is correct (`unsigned char` → 255, +`unsigned short` → 65535, etc.); the special case is only needed for +`float` and `double`. + +**References:** PR #6034 (StructuralSimilarityImageFilter). + +--- + +## 14. clang-tidy Warnings During Refactoring + +### 14a. Do Not Introduce New clang-tidy Diagnostics Refactoring commits should leave the clang-tidy diagnostic count no worse than before. If a refactoring triggers warnings in code it did not touch, those are pre-existing issues and must **not** be fixed in the same commit. Mixing style fixes with behavioral changes obscures commit intent and complicates `git bisect`. -### 12b. clang-tidy Check Families That Conflict with ITK Coding Standards +### 14b. clang-tidy Check Families That Conflict with ITK Coding Standards Several clang-tidy check categories produce warnings that are **incorrect or inapplicable** in an ITK context. Do not "fix" these — doing so either breaks @@ -545,7 +618,7 @@ ITK conventions or introduces irrelevant churn: | `google-*` | Enforces Google style, which differs from ITK naming and formatting rules | | `cppcoreguidelines-avoid-magic-numbers` | ITK uses literal dimension constants (e.g., `2`, `3`) as template arguments | -### 12c. Suppressing False-Positive Checks +### 14c. Suppressing False-Positive Checks For legitimate ITK code that a clang-tidy check incorrectly flags, prefer a `.clang-tidy` config exclusion (`Checks: '-llvmlibc-*'`) over inline @@ -553,7 +626,7 @@ For legitimate ITK code that a clang-tidy check incorrectly flags, prefer a --- -## 13. Quick-Reference Checklist for Refactoring +## 15. Quick-Reference Checklist for Refactoring When refactoring existing code, verify each item: @@ -572,3 +645,5 @@ When refactoring existing code, verify each item: - [ ] Explicit template instantiations in shared-build modules marked `ITK_TEMPLATE_EXPORT` - [ ] ITK deprecated macros replaced (`itkTypeMacro`, `ITK_DISALLOW_COPY_AND_ASSIGN`, `itkStaticConstMacro`) - [ ] No new clang-tidy diagnostics introduced; `llvmlibc-*`, `readability-identifier-length`, and `google-*` warnings ignored if pre-existing +- [ ] `enum class` with `/** */` doxygen comment has a `\class` tag, or use plain `//` comment instead (KWStyle rule) +- [ ] `NumericTraits::min()` is the smallest *positive* value, not the most negative — special-case floating-point dynamic-range defaults diff --git a/Documentation/AI/conventions.md b/Documentation/AI/conventions.md index e1992ad417d..4e3be2b763d 100644 --- a/Documentation/AI/conventions.md +++ b/Documentation/AI/conventions.md @@ -41,6 +41,67 @@ public: itkBooleanMacro(UseSpacing); // Generates UseSpacingOn/Off() ``` +## SWIG `%pythoncode` Brace Forms + +When extending a class with embedded Python in a `.i` file, use the +**verbatim** form `%pythoncode %{ ... %}` for any code that contains +`#` comments: + +```swig +// BAD — `# Foo` is parsed as a SWIG preprocessor directive named "Foo": +%pythoncode { + def __array__(self, dtype=None, copy=None): + # Explicit copy requested. <-- SWIG error + return ... +} + +// GOOD — verbatim block is passed through unchanged: +%pythoncode %{ + def __array__(self, dtype=None, copy=None): + # Explicit copy requested. + return ... +%} +``` + +The single-brace form `%pythoncode { ... }` runs its body through +SWIG's preprocessor and emits errors like: + +``` +Error: Unknown SWIG preprocessor directive: Explicit +``` + +The verbatim `%{ ... %}` form is the safe default for any non-trivial +Python code block. **References:** PR #6027 commit fix. + +## Adding a New Class Checklist + +When adding a new ITK class to a wrapped module (anything under +`Modules/` that has a `wrapping/` subdirectory), every step below is +required to keep CI green: + +1. `include/itkXxx.h` — class declaration, with `\class Xxx` doxygen +2. `include/itkXxx.hxx` — template implementations (header-only) +3. `src/itkXxx.cxx` — non-template implementations + entry in + `src/CMakeLists.txt` +4. `wrapping/itkXxx.wrap` — `itk_wrap_simple_class("itk::Xxx" POINTER)` + (or `itk_wrap_class` + `itk_wrap_image_filter` for templates). + **Missing this file produces `KeyError: 'Xxx'` in Python tests** — + the C++ build still passes, but `ARMBUILD-Python`, + `ITK.Linux.Python`, and `ITK.macOS.Python` will fail. +5. For new `ImageIO` classes: add the factory to `FACTORY_NAMES` in + `itk-module.cmake` *and* register the factory in + `ImageIOFactory.cxx`. +6. New module dependencies → `DEPENDS` (public) or `PRIVATE_DEPENDS` + (implementation-only) in `itk-module.cmake`. +7. `test/itkXxxGTest.cxx` — at least one round-trip / identity test, + plus an exception-validation test. +8. KWStyle: every header has the doxygen `\class Xxx` tag + (see [enforced-code-style.md](./enforced-code-style.md) and + [compiler-cautions.md](./compiler-cautions.md) section 12a). + +**References:** PR #6032 (VTI factory wrapping fix), PR #6034 (SSIM +filter add). + ## Module File Layout ``` diff --git a/Documentation/AI/style.md b/Documentation/AI/enforced-code-style.md similarity index 59% rename from Documentation/AI/style.md rename to Documentation/AI/enforced-code-style.md index 02d59224c83..e2aa9a574f5 100644 --- a/Documentation/AI/style.md +++ b/Documentation/AI/enforced-code-style.md @@ -1,4 +1,8 @@ -# ITK Code Style and Workflow +# ITK Enforced Code Style + +Style rules enforced by pre-commit hooks and CI. Violations block commits +and PRs. See [compiler-cautions.md](./compiler-cautions.md) section 12 for +KWStyle-specific pitfalls. ## First-Time Setup @@ -9,7 +13,7 @@ Configures git hooks (pre-commit clang-format, commit-msg KWStyle check) and remote setup. Required before your first commit. -## C++ Formatting +## C++ Formatting (clang-format) clang-format 19.1.7 is enforced automatically by the pre-commit hook. @@ -18,6 +22,19 @@ Utilities/Maintenance/clang-format.bash --modified # Format modified files onl ``` The hook modifies files in place; re-stage and recommit if it changes anything. +Do not use `--no-verify` to bypass — the format check exists to keep CI green. + +## KWStyle (commit messages and doxygen) + +The `kw-commit-msg.py` hook enforces: +- Subject line ≤78 characters +- Standard prefix required (`ENH:` `BUG:` `COMP:` `DOC:` `STYLE:` `PERF:` `BUILD:`) +- `WIP:` is **not** allowed by `ghostflow-check-main` — use `[WIP]` in the + PR title instead (see [git-commits.md](./git-commits.md)) + +KWStyle also checks that every header has the doxygen `\class` tag. See +[compiler-cautions.md](./compiler-cautions.md) section 12a for the +`enum class` pitfall where KWStyle requires `\class` on enum declarations. ## Naming Conventions @@ -37,18 +54,6 @@ The hook modifies files in place; re-stage and recommit if it changes anything. - Doxygen comments use backslash style: `\class`, `\brief` - No `using namespace` in headers -## Commit Format - -Enforced by `kw-commit-msg.py` hook. Subject line ≤78 characters. - -``` -PREFIX: Brief description - -Longer explanation if needed. -``` - -Prefixes: `ENH:` `BUG:` `COMP:` `DOC:` `STYLE:` `PERF:` `WIP:` - ## CI/CD - **Azure Pipelines** — Linux, Windows, macOS (C++ and Python) diff --git a/Documentation/AI/git-commits.md b/Documentation/AI/git-commits.md index 35e3e274665..22499ce13d9 100644 --- a/Documentation/AI/git-commits.md +++ b/Documentation/AI/git-commits.md @@ -12,6 +12,13 @@ Longer explanation if needed. Describe *what* changed and *why*, not just that a tool made the change. ``` +**The 78-character subject-line limit is a hard constraint, not a guideline.** +The `kw-commit-msg.py` hook rejects commits that exceed it, and CI will +reject PRs containing such commits. Count characters before committing. +The prefix (e.g., `DOC: `) counts toward the 78. When a descriptive +subject would exceed the limit, move detail to the commit body — the +subject should be scannable in `git log --oneline`. + ## Prefixes | Prefix | Use for | @@ -22,7 +29,22 @@ not just that a tool made the change. | `DOC:` | Documentation only | | `STYLE:` | Formatting, naming, no logic change | | `PERF:` | Performance improvement | -| `WIP:` | Work in progress (do not merge) | +| `BUILD:` | Build-system / CMake changes | + +> **Do not use `WIP:` as a commit-subject prefix.** It is not in the +> `ghostflow-check-main` allowed list and will reject the PR. To mark a PR +> as work-in-progress, use a `[WIP]` prefix in the **PR title** (the GitHub +> "WIP" app gates merging on it) but keep individual commit subjects on a +> standard prefix like `ENH:` or `BUG:`. When the PR is ready, remove +> `[WIP]` from the title. + +## Commit Message Length + +Keep commit messages concise. A 1-line subject plus a short body +(≤ ~10 lines) describing *what* and *why* is the norm. Long, essay-style +commit messages have been explicitly objected to in code review — put +expanded rationale in the PR description, not the commit body. The +commit should be readable in `git log --oneline` + a quick `git show`. ## Hook Behavior @@ -36,6 +58,14 @@ them in place if formatting changes are needed. When this happens: Do not use `--no-verify` to bypass hooks — the format check exists to keep CI green. +**All pre-commit hooks must pass before submitting or updating a PR.** +This is non-negotiable. If a hook fails, fix the underlying issue — do +not disable, skip, or work around the hook. A PR with commits that were +not validated by the local hooks will fail CI and waste reviewer time. +If the hook environment is broken (e.g., a stale pixi environment), +fix the environment first: `pixi install` or +`./Utilities/SetupForDevelopment.sh`. + ## First-Time Setup Run once after cloning: @@ -47,12 +77,16 @@ Run once after cloning: This installs the commit-msg and pre-commit hooks. Without it, commits will not be validated locally. -## AI-Assisted Commits +## Commit Attribution + +Commit messages describe **what** changed and **why** — not **how** it +was produced. AI tool names, model IDs, and tool-specific trailers do +**not** belong in commit messages. Disclose substantive AI assistance +in the **PR description** instead (inside a `
` block). -Commit messages for AI-assisted changes must still describe **what** changed -and **why** — not merely that an AI tool generated the change. A -`Co-Authored-By: ` should not be used in commit messages. -See `Documentation/AI/pull-requests.md` for PR-level disclosure requirements. +Full attribution rules (Co-Authored-By, AI disclosure, external +context, PR body format) are in +[attribution.md](./attribution.md). ### Pre-Commit Checklist for C++ Changes diff --git a/Documentation/AI/pull-requests.md b/Documentation/AI/pull-requests.md index 02afc431f08..622a7ce05f4 100644 --- a/Documentation/AI/pull-requests.md +++ b/Documentation/AI/pull-requests.md @@ -15,29 +15,47 @@ to *Ready for Review* until every item below is satisfied. - [ ] The diff has been reviewed for security issues: buffer overflows, deprecated APIs, injection vectors, license conflicts. -## AI Disclosure Requirements +## AI Disclosure and Attribution -State clearly in the PR description how AI tools contributed. Specifically: +See [attribution.md](./attribution.md) for the full policy. Key points: -- Identify which portions are AI-generated and what modifications were made. -- Include evidence of local testing — do not rely on AI assertions of correctness. -- A bare `Co-Authored-By: AI-Tool` trailer is **not** sufficient disclosure. +- **Commits are clean** — no AI tool names, model IDs, or tool-specific + trailers in commit messages. +- **AI disclosure goes in the PR description** — inside a collapsed + `
` block so it doesn't clutter the reviewer's first read. +- **No disclosure needed** for mechanical changes (reformat, rename, + boilerplate). -## PR Description Template +## PR Description Format + +Lead with a **1-3 line visible summary**. Sequester longer analysis, +AI disclosure, test output, and background context inside `
` +blocks. See [attribution.md](./attribution.md) for examples. ```markdown -## Summary +Short summary of what changed and why. Closes #NNNN. + +
+Root cause / design rationale + +Longer explanation here, hidden by default. + +
+ +
+AI assistance - +- Tool, role, what it contributed +- Evidence of local testing -## AI Assistance +
- +
+Test results -## Testing +Commands run, output, baseline comparisons. - +
``` ## Commit Format diff --git a/Documentation/AI/testing.md b/Documentation/AI/testing.md index a706e441d73..4ebbc171c05 100644 --- a/Documentation/AI/testing.md +++ b/Documentation/AI/testing.md @@ -57,3 +57,30 @@ ITK_GTEST_EXERCISE_BASIC_OBJECT_METHODS(ptr, ClassName, SuperclassName); // Set/Get boolean member variable ITK_GTEST_SET_GET_BOOLEAN((&obj), VariableName, value); ``` + +## Exception Tests in GTest Files + +The legacy `ITK_TRY_EXPECT_EXCEPTION` macro from `itkTestingMacros.h` uses +`return EXIT_FAILURE` internally and is **incompatible with GoogleTest**'s +`void TestBody()` — using it produces: + +``` +error: return-statement with a value, in function returning ‘void’ +``` + +In a `*GTest.cxx` file, use vanilla GoogleTest exception macros instead: + +```cpp +// BAD — does not compile in a GTest TEST() body: +ITK_TRY_EXPECT_EXCEPTION(filter->Update()); + +// GOOD: +EXPECT_THROW(filter->Update(), itk::ExceptionObject); +ASSERT_THROW(filter->Update(), itk::ExceptionObject); +``` + +The `ITK_TRY_EXPECT_*` family is still appropriate in legacy +`itkXxxTest.cxx` files that have an `int main()` returning `EXIT_FAILURE` +on error, but never in a Google Test `TEST(...)` block. + +**References:** PR #6034 (StructuralSimilarityImageFilter GTest).