docs: add AI-assisted contribution policy - #606
Conversation
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: matches the description.
TL;DR: One real documentation inconsistency remains in the contributor guide, and the new workflow file is missing the required SPDX header; the two workflow reports are duplicates of the same compliance issue, and no other substantive problems surfaced.
Found 2 high, 2 medium (4 anchored to lines below).
🔴 Critical & high
- Add SPDX license header to compliance.yaml
[high · 96% confidence]in.github/workflows/compliance.yaml:1(details inline) - Missing SPDX license header
[high · 96% confidence]in.github/workflows/compliance.yaml:1(details inline)
🟡 Suggestions
- Missing SPDX header in new workflow file
[medium · 96% confidence]in.github/workflows/compliance.yaml:1(details inline) - Update 'Linear History' section to match CONTRIBUTING.md
[medium · 90% confidence]indocs/hugo/content/en/docs/Development/guidelines.md:60(details inline)
Claim vs. implementation
- Claimed: Add a CONTRIBUTING.md AI-assisted contribution policy that defers to Linux Foundation guidance and adds disclosure/accountability rules, alongside related contribution/compliance docs and checks.
- Done: Adds CONTRIBUTING.md with an AI-assisted contribution section, updates contribution/development docs, and adds CI/pre-commit checks for linear history and stripping notebook outputs.
- Difference: none
How this review was produced
13 specialized finder passes raised 25 findings over the diff and the full changed sources. After de-duplication, 25 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 21 refuted as unsupported, 4 kept (0 tentative).
Refuted by verification:
- Use consistent list numbering style (CONTRIBUTING.md): Markdown ordered lists intentionally use repeated '1.' items here, which is a standard style and not an inconsistency.
- Use consistent shell code block style (CONTRIBUTING.md): The file uses the same fenced code-block language tag 'shell' for both command examples at lines 12 and 18.
- Use consistent shell code block style (CONTRIBUTING.md): The rebase example is also tagged 'shell'; there is no mixed 'bash' versus 'shell' usage in the file.
- Use consistent list numbering style (docs/hugo/content/en/docs/Contribution guidelines/index.md): The markdown intentionally uses repeated '1.' numbering for all ordered-list items, which is valid and renders as a sequence.
- Use consistent shell code block style (docs/hugo/content/en/docs/Development/guidelines.md): The file itself uses a bash code fence at line 59, and the cited inconsistency with CONTRIBUTING.md is outside this file's scope.
- Incorrect Slack channel identifier in link (CONTRIBUTING.md): The Slack-channel claim is unsupported by the file: line 38 contains only the Linux Foundation AI policy link and no Slack link at all.
- Incorrect Slack channel identifier in link (docs/hugo/content/en/docs/Contribution guidelines/index.md): The Slack URL already contains the channel id 'C054GB551TL' in the link target.
- Clarify AI-generated contribution policy for third-party material (CONTRIBUTING.md): Line 38 already states that third-party material in AI output requires permission plus the corresponding notice and attribution.
- Clarify that AI co-author trailers are not permitted in commit messages (CONTRIBUTING.md): Line 42 explicitly says 'Do not add AI co-author trailers to commit messages,' which is a prohibition.
- Use consistent quoting for shell variable expansion in notebook output check (.github/workflows/compliance.yaml): The command already quotes the pathspec and variable expansions in this step:
git diff ... "$BASE" "$HEAD" -- '*.ipynb'. - Incorrect Slack channel link in documentation (CONTRIBUTING.md): There is no Slack channel link in CONTRIBUTING.md; line 38 is about AI policy guidance.
- Incorrect Slack channel link in documentation (docs/hugo/content/en/docs/Contribution guidelines/index.md): The contribution guidelines page links to https://lfenergy.slack.com/archives/C054GB551TL, which matches the stated channel identifier.
- Add missing attribution for LF Energy policy guidance (CONTRIBUTING.md): The referenced policy guidance is already attributed by name and linked on line 38, so no missing attribution notice is evident.
- Add SPDX license header to CONTRIBUTING.md (CONTRIBUTING.md): This new Markdown file is not missing an SPDX header because the project guidelines in the file itself say new files must include a license header, but the finding incorrectly treats that as a required comment header on line 1.
- Add SPDX license header to pre-commit-config.yaml (.pre-commit-config.yaml): The file already has YAML comment lines at the top, but the SPDX-header issue is about the whole new file and the added lines are in scope only from 51 onward.
Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.
| @@ -0,0 +1,65 @@ | |||
| name: Compliance checks | |||
There was a problem hiding this comment.
Add SPDX license header to compliance.yaml
severity: high · confidence: 96%
compliance.yaml is a new GitHub Actions workflow file and must include an SPDX license header in the file's own comment syntax. YAML files use # for comments, so the header should be the first line of the file.
Suggested fix: Add the following SPDX header as the first line of compliance.yaml:
SPDX-FileCopyrightText: 2025 Contributors to DPsim
SPDX-License-Identifier: MPL-2.0
Immediately after the shebang line if present, or as the first line otherwise.
| name: Compliance checks | |
| # SPDX-FileCopyrightText: 2025 Contributors to DPsim | |
| # SPDX-License-Identifier: MPL-2.0 | |
| name: Compliance checks | |
| on: | |
| pull_request: | |
| branches: [master, main] |
Checked against the source: new workflow starts with name: Compliance checks and contains no SPDX comment header
stage: process-compliance
| @@ -0,0 +1,65 @@ | |||
| name: Compliance checks | |||
There was a problem hiding this comment.
Missing SPDX license header
severity: high · confidence: 96%
The new workflow file lacks the required SPDX-License-Identifier and copyright header using the appropriate comment syntax for YAML. Absence of SPDX metadata violates the project's licensing compliance process and may cause downstream license scanning failures.
Suggested fix: Add a YAML comment header at the top of the file with the SPDX lines, e.g.:
SPDX-FileCopyrightText: 2025 Leonardo Carreras leonardo.carreras@eonerc.rwth-aachen.de
SPDX-License-Identifier: MPL-2.0
Place these lines before any other content (or after a shebang if present).
Checked against the source: YAML file begins without the required SPDX license/copyright header
stage: process-compliance
| @@ -0,0 +1,65 @@ | |||
| name: Compliance checks | |||
There was a problem hiding this comment.
Missing SPDX header in new workflow file
severity: medium · confidence: 96%
Documentation states that all new files must include an SPDX license header. The newly added GitHub Actions workflow lacks the required SPDX-FileCopyrightText and SPDX-License-Identifier comments, violating the project’s licensing documentation policy.
Suggested fix: Add an SPDX header as a YAML comment at the top of the file, e.g.,
SPDX-FileCopyrightText: 2025 Leonardo Carreras
SPDX-License-Identifier: MPL-2.0
| name: Compliance checks | |
| # SPDX-FileCopyrightText: 2025 Leonardo Carreras | |
| # SPDX-License-Identifier: MPL-2.0 | |
| name: Compliance checks |
Checked against the source: new workflow file has no SPDX-FileCopyrightText or SPDX-License-Identifier comments anywhere
stage: domain-modeling
| Never merge the target branch into your feature branch — rebase instead: | ||
|
|
||
| ```bash | ||
| git rebase origin/main |
There was a problem hiding this comment.
Update 'Linear History' section to match CONTRIBUTING.md
severity: medium · confidence: 90%
The 'Linear History' section in guidelines.md uses 'main' as the target branch, while CONTRIBUTING.md and the CI workflows consistently use 'master'. This inconsistency can confuse contributors and CI checks.
Suggested fix: Update the example commands in the 'Linear History' section to use 'master' instead of 'main' to align with the rest of the repository and CONTRIBUTING.md.
| git rebase origin/main |
```bash
git rebase origin/master
git push --force-with-lease
```
Checked against the source: guidelines rebase example uses git rebase origin/main while CONTRIBUTING uses upstream/master and PR target master
stage: tests-docs-coverage
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
2c91cdc to
bf835ef
Compare
There was a problem hiding this comment.
DPsim LLM review
Found 1 high, 5 medium (6 anchored to lines below).
🔴 Critical & high
- Incorrect merge‑commit detection in pre‑commit hook
[high · 91% confidence]in.pre-commit-config.yaml:65(details inline)
🟡 Suggestions
- no-merge-commits hook only checks MERGE_HEAD
[medium · 91% confidence]in.pre-commit-config.yaml:65(details inline) - Improve no-merge-commits hook robustness
[medium · 82% confidence]in.pre-commit-config.yaml:65(details inline) - Pin nbconvert version in CI to avoid breakage
[medium · 80% confidence]in.github/workflows/compliance.yaml:56(details inline) - Pin nbconvert version to avoid CI breakage
[medium · 78% confidence]in.github/workflows/compliance.yaml:56(details inline) - Pin nbconvert version in pre-commit hook to avoid breakage
[medium · 78% confidence]in.pre-commit-config.yaml:57(details inline)
How this review was produced
13 specialized finder passes raised 24 findings over the diff and the full changed sources. After de-duplication, 24 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 18 refuted as unsupported, 6 kept (0 tentative).
Refuted by verification:
- Use the project's defined SPDX license identifier consistently (CONTRIBUTING.md): line 43 uses the SPDX identifier text MPL-2.0 and links it to the license URL; this is not an SPDX header/reference defect
- Use DOUBLE_EPSILON for near-zero comparisons in CI scripts (.github/workflows/compliance.yaml): line 53 is a shell empty-string test for NOTEBOOKS, not a numerical near-zero comparison where DOUBLE_EPSILON would apply
- Clarify AI attribution policy for generated content (CONTRIBUTING.md): line 47 explicitly says to disclose in the pull request description when a change is largely generated
- Use consistent quoting in shell steps (.github/workflows/compliance.yaml): line 23 correctly quotes BASE and HEAD in the git log range, and line 24 quotes MERGES in the test
- Clarify AI attribution policy (CONTRIBUTING.md): line 47 explicitly requires saying so in the PR description when a change is largely generated
- Pre-commit hook message formatting (.pre-commit-config.yaml): lines 64-73 use a valid folded YAML shell command and the finding is only a maintainability preference
- Fix inconsistent phrasing about notebook outputs (CONTRIBUTING.md): line 38 is a clear single bullet combining the requirement with the instruction and enforcement mechanism
- Clarify AI co-author trailers in commit messages (CONTRIBUTING.md): line 47 specifically forbids AI co-author trailers while separately stating the sign-off identifies the author
- Typo in step name (.github/workflows/compliance.yaml): line 48 is understandable grammatical wording and does not affect workflow behavior
- Typo in hook name (.pre-commit-config.yaml): lines 61-62 name the hook no-merge-commits and describe the rebase policy, matching the linear-history check intent
- Incorrect logging guideline reference in CONTRIBUTING.md (CONTRIBUTING.md): line 37 is a concise key requirement in CONTRIBUTING.md and does not contradict the development guidelines
- Clarify AI policy reference in AI-assisted contributions section (CONTRIBUTING.md): line 43 provides a direct hyperlink to the Linux Foundation generative AI policy guidance
- Add missing attribution requirement for third-party material in AI-assisted contributions (CONTRIBUTING.md): line 43 says third-party notice and attribution must be supplied alongside the change
- Clarify that pre-commit hook enforcement is automatic only after installation (docs/hugo/content/en/docs/Development/guidelines.md): line 64 already qualifies local enforcement with once you have run pre-commit install
- Add missing SPDX header to CONTRIBUTING.md (CONTRIBUTING.md): lines 1-4 contain a Markdown comment SPDX header with copyright and MPL-2.0 license identifier
Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.
| name: no merge commits (use rebase) | ||
| language: system | ||
| entry: >- | ||
| bash -c '[ ! -f .git/MERGE_HEAD ] || { |
There was a problem hiding this comment.
Incorrect merge‑commit detection in pre‑commit hook
severity: high · confidence: 91%
The no-merge-commits pre‑commit hook checks for the existence of .git/MERGE_HEAD to reject merge commits. This file is only present during an ongoing merge operation and does not indicate that the current branch already contains merge commits. Consequently, the hook will not prevent feature branches that already include merge commits from being committed, violating the linear‑history policy enforced elsewhere (CI job no-merge-commits).
Suggested fix: Replace the check with a command that inspects the branch history for merge commits, e.g., git rev-list --merges --count HEAD or git log --merges --oneline HEAD ^$(git merge-base HEAD main) and fail if any are found.
| bash -c '[ ! -f .git/MERGE_HEAD ] || { | |
| entry: >- | |
| bash -c 'if [ $(git rev-list --merges --count HEAD) -gt 0 ]; then | |
| echo "\nError: merge commits are not allowed in DPsim."; | |
| echo "Please rebase your branch onto the target branch instead:"; | |
| echo "\n git rebase upstream/master"; | |
| echo " git push --force-with-lease"; | |
| echo ""; | |
| exit 1; | |
| fi' |
Checked against the source: the no-merge-commits hook checks only .git/MERGE_HEAD and therefore misses existing merge commits in the branch history
stage: cpp-design
| name: no merge commits (use rebase) | ||
| language: system | ||
| entry: >- | ||
| bash -c '[ ! -f .git/MERGE_HEAD ] || { |
There was a problem hiding this comment.
no-merge-commits hook only checks MERGE_HEAD
severity: medium · confidence: 91%
The local pre-commit hook "no-merge-commits" determines a violation by testing the existence of .git/MERGE_HEAD. This file is present only while a merge is in progress, not when the branch already contains merge commits in its history. Consequently, a developer can commit a branch that includes merge commits and the hook will pass, relying solely on the CI job to catch the issue. This defeats the purpose of the pre‑commit guard and allows merge commits to slip into the repository if CI is bypassed.
Suggested fix: Replace the MERGE_HEAD test with a git log based detection of merge commits in the current branch, analogous to the CI job. For example, run git log --merges $(git merge-base HEAD @{u})..HEAD --oneline and fail if any output is produced.
| bash -c '[ ! -f .git/MERGE_HEAD ] || { | |
| entry: >- | |
| bash -c 'MERGES=$(git log --merges $(git merge-base HEAD @{u})..HEAD --oneline); if [ -n "$MERGES" ]; then | |
| echo ""; | |
| echo "Error: merge commits are not allowed in DPsim."; | |
| echo "Please rebase your branch onto the target branch instead:"; | |
| echo ""; | |
| echo " git rebase upstream/master"; | |
| echo " git push --force-with-lease"; | |
| echo ""; | |
| exit 1; | |
| fi' |
Checked against the source: the local hook determines failure solely from .git/MERGE_HEAD, which is present only during an ongoing merge
stage: io-robustness
| name: no merge commits (use rebase) | ||
| language: system | ||
| entry: >- | ||
| bash -c '[ ! -f .git/MERGE_HEAD ] || { |
There was a problem hiding this comment.
Improve no-merge-commits hook robustness
severity: medium · confidence: 82%
The no-merge-commits pre-commit hook checks for the presence of .git/MERGE_HEAD to detect merge commits. This file is not guaranteed to exist in all Git environments (e.g., shallow clones or CI runners with fetch-depth=1), which can cause the hook to silently pass even when merge commits are present. This undermines the linear-history policy.
Suggested fix: Replace the file-based check with a Git command that directly inspects the commit history, e.g. use 'git merge-base --is-ancestor' or 'git log --merges' to detect merge commits regardless of .git/MERGE_HEAD presence. Update the entry to a shell script that always uses 'git' commands and exits non-zero when merge commits are found.
Checked against the source: the hook only tests for .git/MERGE_HEAD, which detects an in-progress merge rather than merge commits already present in branch history
stage: realtime-resources
| if [ -z "$NOTEBOOKS" ]; then | ||
| exit 0 | ||
| fi | ||
| pip install nbconvert |
There was a problem hiding this comment.
Pin nbconvert version in CI to avoid breakage
severity: medium · confidence: 80%
The CI step installs nbconvert without a pinned version ('pip install nbconvert'). This risks CI failures if nbconvert releases a breaking change or removes a previously available version. Pinning the version ensures reproducibility and aligns with best practices for dependency management in CI.
Suggested fix: Pin nbconvert to a known-good version in the CI workflow, e.g., 'pip install nbconvert==7.16.6'.
| pip install nbconvert | |
| pip install nbconvert==7.16.6 |
Checked against the source: the CI step runs pip install nbconvert without pinning a version
stage: build-deps
| if [ -z "$NOTEBOOKS" ]; then | ||
| exit 0 | ||
| fi | ||
| pip install nbconvert |
There was a problem hiding this comment.
Pin nbconvert version to avoid CI breakage
severity: medium · confidence: 78%
The workflow installs nbconvert without a pinned version, which risks CI failures when a new nbconvert release introduces breaking changes. This violates DPsim's maintainability and reproducibility conventions.
Suggested fix: Pin nbconvert to a known-good version (e.g., 'nbconvert==7.16.4') to ensure the notebook stripping step remains stable across CI runs.
Checked against the source: the workflow installs nbconvert with pip install nbconvert and no version constraint
stage: numerics
| - id: clear-notebook-outputs | ||
| name: clear notebook outputs | ||
| language: python | ||
| additional_dependencies: ["nbconvert"] |
There was a problem hiding this comment.
Pin nbconvert version in pre-commit hook to avoid breakage
severity: medium · confidence: 78%
The pre-commit hook declares nbconvert as an additional dependency without a pinned version ('additional_dependencies: ["nbconvert"]'). This risks breakage if nbconvert releases a breaking change. Pinning the version ensures the hook remains stable across environments.
Suggested fix: Pin nbconvert to the same version used in CI, e.g., 'additional_dependencies: ["nbconvert==7.16.6"]'.
Checked against the source: additional_dependencies lists nbconvert without a version pin
stage: build-deps
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| BASE=${{ github.event.pull_request.base.sha }} | ||
| HEAD=${{ github.event.pull_request.head.sha }} | ||
| NOTEBOOKS=$(git diff --name-only --diff-filter=d "$BASE" "$HEAD" -- '*.ipynb') | ||
| if [ -z "$NOTEBOOKS" ]; then |
| BASE=${{ github.event.pull_request.base.sha }} | ||
| HEAD=${{ github.event.pull_request.head.sha }} | ||
| NOTEBOOKS=$(git diff --name-only --diff-filter=d "$BASE" "$HEAD" -- '*.ipynb') | ||
| if [ -z "$NOTEBOOKS" ]; then |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #606 +/- ##
==========================================
- Coverage 72.75% 72.45% -0.30%
==========================================
Files 497 497
Lines 32305 32305
Branches 17470 17470
==========================================
- Hits 23503 23408 -95
- Misses 8801 8896 +95
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Contributors are already using AI assistants and the project says nothing about it, which leaves both the licensing question and the accountability question to be settled ad hoc in review. This adds a short section to CONTRIBUTING.md.
It defers to the Linux Foundation policy guidance on generative AI tools rather than inventing a position. That guidance permits AI-generated contributions and states what a contributor must establish first, of which two requirements matter here: the tool's terms must not restrict its output in ways that conflict with the MPL-2.0, and where the output carries pre-existing third-party material the contributor needs permission to contribute it and must supply the corresponding notice and attribution. LF Energy publishes nothing further, so that guidance governs, and it explicitly invites projects to add their own on top.
What this project adds is accountability and disclosure. The contributor is the author of whatever they submit and should not open a pull request containing code they cannot explain, since the sign-off is a claim under the DCO that they have the right to submit the work. Substantially generated changes should say so in the pull request description so reviewers know where to look harder, and commits should not carry AI co-author trailers, because the sign-off already identifies the author. Numerical results and validation notebooks get the same scrutiny as code, plausible output not being evidence a model is correct. Unpublished research, confidential data and third-party code should not be pasted into an external service.
Depends on #509, which creates the file.