Skip to content

DOC: Add Create a Pull Request section with draft PR guidance#5975

Open
blowekamp wants to merge 2 commits intoInsightSoftwareConsortium:mainfrom
blowekamp:docs_pr_guidelines
Open

DOC: Add Create a Pull Request section with draft PR guidance#5975
blowekamp wants to merge 2 commits intoInsightSoftwareConsortium:mainfrom
blowekamp:docs_pr_guidelines

Conversation

@blowekamp
Copy link
Copy Markdown
Member

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

@github-actions github-actions bot added type:Documentation Documentation improvement or change area:Documentation Issues affecting the Documentation module labels Mar 20, 2026
Copy link
Copy Markdown
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

💯 🥇 thanks, @blowekamp !

@N-Dekker
Copy link
Copy Markdown
Contributor

Cool, @blowekamp Very glad to have such a guidance for PRs! 👍

Some more detailed comments coming up...

author has personally verified that:

- all automated CI tests pass,
- the implementation is correct and complete,
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker Mar 21, 2026

Choose a reason for hiding this comment

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

Can you please make this:

- the implementation is correct, complete and fully understood,

I think it's essential that we keep understanding the code, even when it is partially AI generated, to avoid cognitive debt. As I mentioned at https://discourse.itk.org/t/ai-generated-pull-requests-overwhelming-hard-to-review-carefully/7728/12?u=niels_dekker

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.

I am concerned that the new developer burden to meet all the requirements of contributing to ITK may discourage participation. I understand the intent of this request, but the tone may need to be more inviting to new developers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand the intent of this request, but the tone may need to be more inviting to new developers.

@hjmjohnson You mean like replacing words like "must" with more friendly and modest expressions like "please do..." or "we prefer to...", right?

Copy link
Copy Markdown
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Good as-is. The feedback provided so far also looks useful.

@N-Dekker
Copy link
Copy Markdown
Contributor

N-Dekker commented Mar 27, 2026

I guess many organizations worldwide are writing guidelines for AI assisted PRs, these days. Did you write your PR entirely from scratch, or did you get some inspiration from others?

This one looks quite nice as well: How to Be an Open Source Hero: Contributing AI-Generated Code with Care by Jake Levirne and Antaripa Saha, DigitalOcean, Oct 10, 2025


Another example:

@hjmjohnson
Copy link
Copy Markdown
Member

I've incorporated the requested language changes in https://github.com/hjmjohnson/ITK/tree/docs_pr_guidelines — here's a summary of what was applied:

  1. Softened tone (addressing @hjmjohnson's concern about discouraging new contributors and @N-Dekker's suggestion to replace "must" with friendlier phrasing): **AI-agent-assisted pull requests must be opened...** An agent-created PR must not be converted**Please open AI-agent-assisted pull requests in Draft mode.** We prefer that an agent-created PR not be converted

  2. Added "fully understood" (@N-Dekker line 251): the implementation is correct and complete,the implementation is correct, complete and fully understood,

  3. Concrete reviewer wait time (@N-Dekker + @hjmjohnson): Replaced "Do not request a review before these conditions are met." with "Please allow at least 1 working day for low-impact changes (e.g., single test changes, documentation updates) and longer for changes that impact core infrastructure or fundamental algorithmic behavior before merging."

  4. Skipped the AI prompt inclusion request — per @thewtex's reasoning that this is burdensome and adds noise rather than value.

@blowekamp — feel free to cherry-pick or incorporate these suggestions directly.

@blowekamp blowekamp marked this pull request as ready for review March 27, 2026 17:59
@blowekamp blowekamp marked this pull request as draft March 27, 2026 17:59
@blowekamp
Copy link
Copy Markdown
Member Author

@hjmjohnson Please just force push to my branch to update with your changes. Thanks for following up I was unsure the best way to address everything.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 27, 2026

Greptile Summary

This PR adds a new "Create a Pull Request" section to the contributing guide, establishing a clear draft-first workflow for all contributors and additional review requirements specifically for AI-agent-assisted PRs.

  • Splits the prior one-liner about the PR URL into a dedicated, anchored (create-a-pr) section with step-by-step guidance.
  • Advises all contributors to open PRs as Drafts until CI passes, then convert to Ready for Review.
  • Adds a {important} callout mandating that AI-agent-assisted PRs remain in Draft until the human author has verified CI success, correctness, and an accurate description.
  • Includes a reviewer-time guideline (≥1 working day for low-impact changes, longer for core/algorithmic changes) before merging, scoped to the AI-agent callout.
  • All MyST Markdown syntax (anchors, {important} directive, RST-style section headings) is correct and consistent with the rest of the file.

Confidence Score: 5/5

This is a documentation-only PR with no code changes; it is safe to merge.

All changes are purely additive documentation. The MyST Markdown syntax is correct, the guidance is consistent with the surrounding file style, and the prior terminology concern raised in an earlier thread has been addressed. No P0 or P1 issues found.

No files require special attention.

Important Files Changed

Filename Overview
Documentation/docs/contributing/index.md Adds a new "Create a Pull Request" section with draft PR guidance and an AI-agent-specific important callout covering review time expectations; changes are well-structured and use correct MyST Markdown syntax.

Sequence Diagram

sequenceDiagram
    participant Dev as Developer / AI Agent
    participant Git as git review-push
    participant GH as GitHub PR
    participant CI as CI / Azure Pipelines
    participant Rev as Reviewer / Maintainer

    Dev->>Git: git review-push --force
    Git-->>Dev: URL provided in terminal
    Dev->>GH: Open PR as Draft
    GH->>CI: Trigger automated tests
    CI-->>GH: Tests pass / fail

    alt Tests fail
        CI-->>Dev: Notify failure
        Dev->>Git: Fix & re-push (Revise a Topic)
    end

    alt AI-agent-assisted PR
        Note over Dev,GH: Human author must verify:<br/>1. CI tests pass<br/>2. Implementation correct & understood<br/>3. PR description accurate
        Dev->>GH: Convert Draft → Ready for Review
        Note over GH,Rev: Allow ≥1 working day (low-impact)<br/>or longer (core/algorithmic changes)<br/>before merging
    else Standard PR
        Dev->>GH: Convert Draft → Ready for Review
        Dev->>Rev: Request reviewers
    end

    Rev->>GH: Review & approve
    Rev->>GH: Merge PR
Loading

Reviews (2): Last reviewed commit: "DOC: Soften tone and incorporate reviewe..." | Re-trigger Greptile

@N-Dekker
Copy link
Copy Markdown
Contributor

4. Skipped the AI prompt inclusion request — per thewtex's reasoning that this is burdensome and adds noise rather than value.

OK, I understand that it can be burdensome when there are many prompts and agent settings involved. However, I still think we should encourage PR authors to share the essential information on how the PR was generated. The DigitalOcean article says about it:

Key Takeaways
Be transparent. Disclose how AI contributed to your pull request and link process context when you can.

and

If You’re a Contributor
Disclose your tools and how you used them

Just like we share the regular expression with a PR that proposes a large find-and-replace code change.

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Mar 27, 2026

However, I still think we should encourage PR authors to share the essential information on how the PR was generated.

Just like we share the regular expression with a PR that proposes a large find-and-replace code change.

I agree, in general, that we should share essential information on how and why a PR was generated. Including a regular expression that was applied is a great example.

DigitalOcean article says about it:

This article, from October 2025, is extremely old in terms of development practices, speaking for myself, at least. A single prompt, "I used ChatGPT", or "I used Claude Code" does not capture or really encompass how a PR is created. There is an entire disciple called "harness engineering" now. And there is a lot more involved. I do not think we need developers to try to include what text editor they used, their shell, one of the LLMs in one of the harness agents, etc. It is too much and it does not really add value.

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Mar 27, 2026

Did you write your PR entirely from scratch, or did you get some inspiration from others?

A side note, this was discussed at the most recent Insight Software Consortium Council meeting.

@hjmjohnson hjmjohnson marked this pull request as ready for review March 27, 2026 23:27
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.

5 participants