Skip to content

DOC: Capture recent CI lessons in Documentation/AI/ skill files#6040

Open
hjmjohnson wants to merge 2 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson:doc-ai-skill-updates-clean
Open

DOC: Capture recent CI lessons in Documentation/AI/ skill files#6040
hjmjohnson wants to merge 2 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson:doc-ai-skill-updates-clean

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson commented Apr 10, 2026

Update Documentation/AI/ skill files with CI lessons learned and revised attribution rules based on community feedback. Closes #6055.

Commit 1: CI lessons and code-review patterns
  • git-commits.md, style.md — drop WIP: prefix, add BUILD:, document [WIP] PR-title workflow
  • compiler-cautions.md — KWStyle enum class pitfall (§12), NumericTraits float range (§13)
  • testing.md — GTest exception macros (ITK_TRY_EXPECT_EXCEPTION incompatible with GoogleTest)
  • attribution.md — initial attribution rules
  • code-review-lessons.md — new file capturing recurring review patterns
Commit 2: Revise attribution rules per #6055 feedback

Incorporates feedback from N-Dekker (#6055 comment), blowekamp (Discourse #7728 post #16), and dzenanz (#6055 comment):

  • Remove AI trailers from commitsTool-Assisted: and Assisted-by: trailers removed. AI disclosure moves entirely to PR description inside <details> blocks.
  • Co-Authored-By: for AI → "discouraged" (was "never") — acknowledges N-Dekker's GitHub-linking-semantics argument while noting blowekamp's preference for no AI info in commits.
  • Reviewer vs co-author distinctionCo-Authored-By: only for design-shaping feedback, not routine review. Per N-Dekker: "A reviewer may not completely agree with the PR... A co-author usually does."
  • Stronger brevity guidance — PR descriptions must lead with 1-3 line visible summary; all longer analysis in collapsed <details> blocks. Per dzenanz: "commit messages and PR body should be concise."
  • PR description is sole AI disclosure mechanism — commits are clean.

@github-actions github-actions bot added type:Documentation Documentation improvement or change area:Documentation Issues affecting the Documentation module labels Apr 10, 2026
@hjmjohnson hjmjohnson closed this Apr 10, 2026
@hjmjohnson hjmjohnson deleted the doc-ai-skill-updates-clean branch April 10, 2026 19:07
@hjmjohnson hjmjohnson restored the doc-ai-skill-updates-clean branch April 10, 2026 19:15
@hjmjohnson hjmjohnson reopened this Apr 10, 2026
@hjmjohnson hjmjohnson marked this pull request as ready for review April 10, 2026 21:07
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 10, 2026

Greptile Summary

Pure documentation update to five Documentation/AI/*.md skill files, capturing lessons from recent CI failures across PRs #6027#6035. Changes cover: dropping WIP: commit prefix in favour of BUILD: and [WIP] PR-title workflow; KWStyle's enum class doxygen requirement; NumericTraits<float>::min() float-range semantics; SWIG %pythoncode %{…%} verbatim syntax; a new-class CI checklist; and GTest-compatible exception macros.

  • The clang-tidy sub-sections (### 12a, ### 12b, ### 12c) inside the newly-renumbered §14 were not updated to ### 14a/14b/14c, leaving a numbering mismatch that could confuse an agent cross-referencing by number.

Confidence Score: 4/5

Safe to merge after fixing the stale sub-section numbers in §14 of compiler-cautions.md.

One P1 documentation inconsistency: sub-sections 12a/12b/12c inside the renumbered §14 (clang-tidy) were not updated. Since this file is used as a reference guide by AI agents, the numbering mismatch is a real (if minor) correctness issue for its intended consumers. All other changes are accurate and well-structured.

Documentation/AI/compiler-cautions.md — sub-section labels in §14 still carry old §12 numbers.

Important Files Changed

Filename Overview
Documentation/AI/compiler-cautions.md Adds §12 (KWStyle/Doxygen) and §13 (NumericTraits float range), renumbers old §12→14 and §13→15; sub-section labels 12a/12b/12c inside the newly-renamed §14 were not updated and remain stale.
Documentation/AI/conventions.md Adds SWIG %pythoncode verbatim-form guidance and a 9-point new-class checklist; accurate and well-structured.
Documentation/AI/git-commits.md Removes WIP: prefix, adds BUILD:, documents [WIP] PR-title workflow, and adds commit-length guidance; all correct.
Documentation/AI/style.md Updates prefix list to replace WIP: with BUILD: and adds a cross-reference note; clean change.
Documentation/AI/testing.md Adds a GTest-compatible exception-test section with correct EXPECT_THROW/ASSERT_THROW guidance; accurate.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[New ITK class or commit] --> B{Commit prefix check}
    B -->|WIP: used| C[ghostflow-check-main REJECTS]
    B -->|ENH or BUG or BUILD etc.| D[ghostflow OK]
    D --> E{enum class with doxygen comment?}
    E -->|no backslash-class tag| F[KWStyle CI FAILS]
    E -->|plain comment or backslash-class tag| G[KWStyle OK]
    G --> H{GTest exception test?}
    H -->|ITK_TRY_EXPECT_EXCEPTION| I[Compile error in TestBody]
    H -->|EXPECT_THROW or ASSERT_THROW| J[GTest OK]
    J --> K{Python wrapping?}
    K -->|Missing itkXxx.wrap| L[Python CI KeyError]
    K -->|wrap file present| M[CI GREEN]
    K -->|pythoncode single-brace with hash comment| N[SWIG preprocessor error]
    K -->|pythoncode verbatim form| M
Loading

Comments Outside Diff (1)

  1. Documentation/AI/compiler-cautions.md, line 570-592 (link)

    P1 Stale sub-section numbers after renumbering

    Section 12 (clang-tidy) was promoted to section 14, but its three sub-sections (### 12a, ### 12b, ### 12c) were not updated to ### 14a, ### 14b, ### 14c. An agent loading this file and following the numbering will see a mismatch: the parent heading says 14 while the children still say 12.

Reviews (1): Last reviewed commit: "DOC: Capture recent CI lessons in Docume..." | Re-trigger Greptile

Comment thread Documentation/AI/compiler-cautions.md Outdated
@hjmjohnson hjmjohnson force-pushed the doc-ai-skill-updates-clean branch 5 times, most recently from 968d8ee to 9e9d8d5 Compare April 12, 2026 12:25
@blowekamp
Copy link
Copy Markdown
Member

I have been using CMakeUSerPresets.json to hold predefined configuration of ITK such as "development", "wrapping", "system-libraries", or "doxygen". Currently I have some local instructions in .github-instructions.md

Specifically I have:


## Build and Testing

DO NOT USE pixie for development.

Use the CMakePresets.json and CMakeUserPresets.json files to configure the build environment: `cmake --preset PRESET_NAME` to configure the build and `cmake --build --preset PRESET_NAME` to build.

The full compilation of ITK and it's test suit is time consuming. Building specific targets can speed the development process.

To build targets `cmake --build --preset PRESET_NAME --target TARGET` should be used.

Each module name is also a build target for the module which include tests targets.

The test drivers are named such as ITKCommon1TestDriver, ITKCommonGTestDriver, or ITKIOImageBaseTestDriver.

Tests should be run with the `ctest --preset PRESET_NAME` command.

I have found these CMakePreset.json files very useful to manage different configurations, perhaps they could be more widely adopted? Not sure how user customization of these instructions should occur.

@hjmjohnson
Copy link
Copy Markdown
Member Author

@blowekamp Are you suggesting changes to the proposed updates?

@blowekamp
Copy link
Copy Markdown
Member

@hjmjohnson No. Just a question/conversation relate to these changes.

  • What is the recommended way for local instructions on building or other local customization?
  • Would adopting CMakePresets[User].json into the build/testing instruction be widely useful?

@hjmjohnson
Copy link
Copy Markdown
Member Author

hjmjohnson commented Apr 13, 2026

@hjmjohnson No. Just a question/conversation related to these changes.

  • What is the recommended way for local instructions on building or other local customization?
  • Would adopting CMakePresets[User].json into the build/testing instruction be widely useful?

@blowekamp Ahh... I see. My opinion "CMakePresets[User].json into the build/testing instruction be widely" would be widely useful. I've had some success with it, and I think it would be a way to codify, in an AI-friendly JSON format, more meaning than can be achieved with mixes of shell command-line options. I've made a note to make a WIP to promote discussion after some of my other open PR's have cleared.

@hjmjohnson hjmjohnson force-pushed the doc-ai-skill-updates-clean branch 3 times, most recently from 12eedae to c2ea712 Compare April 14, 2026 12:51
@hjmjohnson hjmjohnson requested review from blowekamp and dzenanz April 14, 2026 16:26
@hjmjohnson hjmjohnson force-pushed the doc-ai-skill-updates-clean branch from c2ea712 to bab0248 Compare April 14, 2026 17:19
@hjmjohnson
Copy link
Copy Markdown
Member Author

@blowekamp @N-Dekker @thewtex @dzenanz I think these are in a reasonable state to consider now. After these updates, Greptile will be able to better enforce the "historical community perspectives" as represented by these rules extracted from various commit, PR-comment, and discourse comments.

Comment thread Documentation/AI/git-commits.md
@hjmjohnson hjmjohnson force-pushed the doc-ai-skill-updates-clean branch from bab0248 to cb73c5a Compare April 14, 2026 20:18
@hjmjohnson hjmjohnson requested a review from N-Dekker April 14, 2026 20:20
Comment thread Documentation/AI/attribution.md Outdated
Comment thread Documentation/AI/attribution.md
Comment on lines +198 to +199
## 10. ITK Initializer Patterns

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@N-Dekker does this look good to you?

Comment thread Documentation/AI/git-commits.md
@hjmjohnson hjmjohnson force-pushed the doc-ai-skill-updates-clean branch from cb73c5a to 0a91cb6 Compare April 14, 2026 22:14
…ion/AI/

Add and expand AI agent guidance files based on lessons from recent PRs
(ccache fixes, compiler flag removal, backports, attribution policy):

- attribution.md: commit/PR attribution rules for AI-assisted work
- code-review-lessons.md: patterns from 50+ reviewed PRs
- compiler-cautions.md: new sections on unused-variable warnings,
  declare-then-assign pitfalls, refactoring checklist
- conventions.md: ITK class/CMake/wrapping conventions
- enforced-code-style.md: renamed from style.md, expanded
- git-commits.md: hook enforcement, pre-commit checklist, deduplication
  with attribution.md (now cross-references instead of duplicating)
- pull-requests.md: AI disclosure requirements
- testing.md: targeted ctest patterns
- AGENTS.md: updated routing table for new/renamed files
@hjmjohnson hjmjohnson force-pushed the doc-ai-skill-updates-clean branch from 0a91cb6 to d6c9dc0 Compare April 14, 2026 22:16
@hjmjohnson hjmjohnson requested a review from thewtex April 14, 2026 22:17
…community feedback

Incorporate feedback from N-Dekker, blowekamp, and dzenanz:

- Remove Tool-Assisted: and Assisted-by: trailers from commit
  guidance — AI disclosure moves entirely to the PR description
  inside <details> blocks (blowekamp's position)
- Change Co-Authored-By: for AI from "never" to "discouraged"
  with rationale, acknowledging the open question (N-Dekker's
  GitHub-linking-semantics argument)
- Distinguish reviewer Co-Authored-By: (design-shaping feedback)
  from review acknowledgment (prose mention) per N-Dekker's
  reviewer-vs-coauthor distinction
- Strengthen brevity guidance: PR descriptions use collapsed
  <details> sections by default, keeping only 1-3 line summary
  visible (dzenanz's conciseness feedback)
- Make PR description the primary and sole mechanism for AI
  disclosure, with commit messages containing no AI attribution

Closes InsightSoftwareConsortium#6055
@hjmjohnson hjmjohnson force-pushed the doc-ai-skill-updates-clean branch from aebbf21 to 1aea1f9 Compare April 15, 2026 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Documentation Issues affecting the Documentation module type:Documentation Documentation improvement or change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DOC: Define commit message attribution rules for AI-assisted contributions

4 participants