docs: add CONTRIBUTING.md and a notebook output compliance check - #509
docs: add CONTRIBUTING.md and a notebook output compliance check#509leonardocarreras wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #509 +/- ##
==========================================
+ Coverage 72.91% 72.95% +0.03%
==========================================
Files 497 497
Lines 32398 32398
Branches 17550 17551 +1
==========================================
+ Hits 23623 23635 +12
- Misses 8760 8762 +2
+ Partials 15 1 -14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a89e7b3 to
0cd51f0
Compare
19656dc to
025b480
Compare
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: matches the description.
Found 5 high, 3 medium (7 anchored to lines below).
🔴 Critical & high
- Add SPDX license header to CONTRIBUTING.md
[high · 97% confidence]inCONTRIBUTING.md:1(details inline) - Add SPDX license header to compliance.yaml
[high · 97% confidence]in.github/workflows/compliance.yaml:1(details inline) - no-merge-commits pre-commit hook only checks MERGE_HEAD
[high · 90% confidence]in.pre-commit-config.yaml:65(details inline) - Hook name misleads about its behavior
[high · 86% confidence]in.pre-commit-config.yaml:65(details inline)
🟡 Suggestions
- Missing SPDX license header
[medium · 97% confidence]in.github/workflows/compliance.yaml:1(details inline) - Incorrect merge‑commit detection in pre‑commit hook
[medium · 90% confidence]in.pre-commit-config.yaml:65(details inline) - no-merge-commits pre-commit hook only checks ongoing merge
[medium · 90% confidence]in.pre-commit-config.yaml:65(details inline)
🔵 Optional / low-confidence (1)
- Inconsistent branch name in rebase instructions
[high · 35% confidence · unconfirmed]inCONTRIBUTING.md:20
Claim vs. implementation
- Claimed: Add CONTRIBUTING.md plus CI and pre-commit checks to enforce linear history, and update docs with rebase and notebook-output guidance.
- Done: Adds a root CONTRIBUTING.md, a PR workflow that blocks merge commits and saved notebook outputs, a pre-commit hook for merge commits and notebook clearing, and updates contribution/development docs with rebase and notebook-output instructions.
- Difference: none
How this review was produced
13 specialized finder passes raised 38 findings over the diff and the full changed sources. After de-duplication, 36 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 28 refuted as unsupported, 8 kept (1 tentative).
Refuted by verification:
- Use consistent terminology for development tooling (CONTRIBUTING.md): Line 8 only describes pre-commit install activating automated checks; no hook-name mismatch is present in this file.
- Use DOUBLE_EPSILON for tolerance in notebook output check (.github/workflows/compliance.yaml): Line 54 is the nbconvert command; there is no DOUBLE_EPSILON tolerance logic or related comparison in this workflow.
- Clarify nbconvert entry point in pre-commit hook (.pre-commit-config.yaml): The hook entry is present exactly at line 58; the finding asks for an extra comment, which is a style suggestion rather than a defect.
- Improve error message formatting in no-merge-commits hook (.pre-commit-config.yaml): The multi-echo error block is already implemented in the hook entry at lines 65-73, so the claimed formatting issue is not a missing change.
- Use consistent numbering style in ordered lists (CONTRIBUTING.md): All ordered-list items use the same Markdown form
1.throughout the Quick start section. - Use consistent numbering style in ordered lists (docs/hugo/content/en/docs/Contribution guidelines/index.md): The file uses the same Markdown ordered-list marker '1.' on every item, which is valid and consistent.
- Use consistent numbering style in ordered lists (docs/hugo/content/en/docs/Development/guidelines.md): The added ordered lists consistently use
1.for every item in the changed section. - Use consistent list numbering style (CONTRIBUTING.md): The Quick start list is consistently numbered with
1.on every item, which is valid Markdown. - Use consistent list numbering style (docs/hugo/content/en/docs/Contribution guidelines/index.md): All list items in the Quick start section are formatted uniformly as '1.'; there is no mixed numbering style in the file.
- Incorrect rebase command example (docs/hugo/content/en/docs/Development/guidelines.md): The file explicitly says
git rebase origin/main, so the example is present as written. - Typo in code block comment (docs/hugo/content/en/docs/Development/guidelines.md): The phrase
DPsim currently uses tois in the file unchanged, but this is pre-existing text outside the changed lines. - Fix inconsistent numbering in quick-start list (CONTRIBUTING.md): Markdown ordered lists may intentionally use repeated
1.; the file does so consistently in Quick start. - Fix inconsistent numbering in key requirements list (CONTRIBUTING.md): Lines 28-34 are a bullet list, not an enumerated list, so the numbering complaint does not apply.
- Fix inconsistent numbering in quick-start list (docs/hugo/content/en/docs/Contribution guidelines/index.md): The enumerated list is intentionally written with repeated '1.' markers, which Markdown renders correctly as a numbered list.
- Section title mismatch between guidelines and rendered heading (docs/hugo/content/en/docs/Development/guidelines.md): The source heading is
1. **Linear History (no merge commits)**and there is no separate rendered-heading mismatch in the file.
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,38 @@ | |||
| # Contributing to DPsim | |||
There was a problem hiding this comment.
Add SPDX license header to CONTRIBUTING.md
severity: high · confidence: 97%
CONTRIBUTING.md is a new documentation file and must include an SPDX license header in the correct syntax for Markdown files (# SPDX-...). The absence of the header violates the project's SPDX header policy for new files.
Suggested fix: Add the SPDX license header at the top of the file, after the UTF-8 BOM if present, using the correct Markdown comment syntax:
SPDX-License-Identifier: MPL-2.0
Contributing to DPsim
| # Contributing to DPsim | |
| # SPDX-License-Identifier: MPL-2.0 | |
| # Contributing to DPsim |
Checked against the source: the new Markdown file begins with the document title and has no SPDX license or copyright header
stage: process-compliance
| @@ -0,0 +1,65 @@ | |||
| name: Compliance checks | |||
There was a problem hiding this comment.
Add SPDX license header to compliance.yaml
severity: high · confidence: 97%
compliance.yaml is a new GitHub Actions workflow file and must include an SPDX license header in the correct syntax for YAML files (# SPDX-License-Identifier: MPL-2.0). The absence of the header violates the project's SPDX header policy for new files.
Suggested fix: Add the SPDX license header at the top of the file using the correct YAML comment syntax:
SPDX-License-Identifier: MPL-2.0
name: Compliance checks
| name: Compliance checks | |
| # SPDX-License-Identifier: MPL-2.0 | |
| name: Compliance checks |
Checked against the source: the new workflow starts with name: Compliance checks and contains no SPDX license or copyright header
stage: process-compliance
| 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 pre-commit hook only checks MERGE_HEAD
severity: high · confidence: 90%
The local pre-commit hook "no-merge-commits" determines a linear history violation by testing the existence of the file .git/MERGE_HEAD. This file is present only during an in‑progress merge operation, not after a merge commit has been created. Consequently, branches that already contain merge commits will pass the pre‑commit check, violating the documented linear‑history policy and allowing prohibited merge commits to be introduced if CI is bypassed.
Suggested fix: Replace the MERGE_HEAD check with a git log query that detects merge commits in the current branch history, similar to the CI workflow. The hook should abort when any merge commit is found between the branch tip and the upstream base.
| bash -c '[ ! -f .git/MERGE_HEAD ] || { | |
| entry: >- | |
| bash -c ' | |
| BASE=$(git rev-parse --abbrev-ref HEAD); | |
| UPSTREAM=$(git rev-parse @{u} 2>/dev/null || echo origin/main); | |
| MERGES=$(git log --merges --oneline ${UPSTREAM}..${BASE}); | |
| 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 origin/main"; | |
| echo " git push --force-with-lease"; | |
| echo ""; | |
| exit 1; | |
| fi | |
| ' |
Checked against the source: the hook entry tests only for .git/MERGE_HEAD, which detects an in-progress merge rather than existing merge commits
stage: domain-modeling
| name: no merge commits (use rebase) | ||
| language: system | ||
| entry: >- | ||
| bash -c '[ ! -f .git/MERGE_HEAD ] || { |
There was a problem hiding this comment.
Hook name misleads about its behavior
severity: high · confidence: 86%
The pre‑commit hook is named "no-merge-commits" and its description says "no merge commits (use rebase)", but the script only checks for the presence of a MERGE_HEAD file (i.e. an ongoing merge) and does not detect existing merge commits in the branch history. This discrepancy can give contributors a false sense of safety, as merge commits will still pass the pre‑commit check while the CI workflow correctly blocks them.
Suggested fix: Either rename the hook to reflect its actual check (e.g. "no-merge-in-progress") or implement a proper detection of merge commits, similar to the CI workflow (e.g. using git log --merges).
Checked against the source: despite the no-merge-commits name, the implemented check is only the MERGE_HEAD file test
stage: naming-docs
| @@ -0,0 +1,65 @@ | |||
| name: Compliance checks | |||
There was a problem hiding this comment.
Missing SPDX license header
severity: medium · confidence: 97%
New workflow file lacks required SPDX-License-Identifier and copyright header using the YAML comment syntax. This violates the project's licensing compliance process and may cause downstream legal or CI checks to fail.
Suggested fix: Add a SPDX header at the top of the file using '#'. Include both the copyright notice and the MPL-2.0 license identifier.
| name: Compliance checks | |
| # SPDX-FileCopyrightText: 2025 Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de> | |
| # SPDX-License-Identifier: MPL-2.0 |
Checked against the source: the new YAML workflow lacks the required SPDX-License-Identifier and copyright comments at the top
stage: process-compliance
| 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: medium · confidence: 90%
The no-merge-commits pre‑commit hook checks for the existence of .git/MERGE_HEAD (line 65). 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 reject branches that have merge commits in their history, violating the linear‑history policy described in the documentation and CI workflow.
Suggested fix: Replace the check with a git command that inspects the commit history for merge commits, e.g. git log --merges --oneline $(git merge-base HEAD origin/main)..HEAD. Abort if any output is found.
| bash -c '[ ! -f .git/MERGE_HEAD ] || { | |
| entry: >- | |
| bash -c 'MERGES=$(git log --merges --oneline $(git merge-base HEAD origin/main)..HEAD); if [ -n "$MERGES" ]; then echo "\nError: merge commits are not allowed in DPsim."; echo "Please rebase your branch onto the target branch instead:"; echo "\n git rebase origin/main"; echo " git push --force-with-lease"; echo "\n"; exit 1; fi' |
Checked against the source: the no-merge-commits hook uses [ ! -f .git/MERGE_HEAD ] and does not inspect 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 pre-commit hook only checks ongoing merge
severity: medium · confidence: 90%
The hook defined at line 64 uses [ ! -f .git/MERGE_HEAD ] to detect merge commits. This only catches an in‑progress merge operation, not merge commits that already exist in the branch history. Consequently, a developer can commit a branch that already contains merge commits, violating the linear‑history policy enforced by CI.
Suggested fix: Replace the current entry with a script that scans the branch history for merge commits (e.g. using git rev-list --merges between the branch base and HEAD) and fails if any are found.
| bash -c '[ ! -f .git/MERGE_HEAD ] || { | |
| entry: >- | |
| bash -c ' | |
| BASE=$(git merge-base HEAD origin/main) | |
| MERGES=$(git rev-list --merges --count "$BASE"..HEAD) | |
| if [ "$MERGES" -gt 0 ]; 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 origin/main"; | |
| echo " git push --force-with-lease"; | |
| echo ""; | |
| exit 1; | |
| fi' |
Checked against the source: the changed hook entry checks .git/MERGE_HEAD only, so already-created merge commits are not detected by this hook
stage: io-robustness
063ce4c to
785f9ce
Compare
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: The notebook CI check is not scoped to notebooks touched by the PR as claimed; it clears outputs on the touched notebooks and then fails if any diff remains, which can also be affected by the strip operation itself.
Found 3 high, 11 medium (8 anchored to lines below).
🔴 Critical & high
- No-merge-commits pre-commit hook does not detect existing merge commits
[high · 86% confidence]in.pre-commit-config.yaml:65(details inline) - Ineffective no‑merge‑commits pre‑commit hook
[high · 86% confidence]in.pre-commit-config.yaml:65(details inline)
🟡 Suggestions
- Pin nbconvert version to avoid future breakage
[medium · 88% confidence]in.pre-commit-config.yaml:57(details inline) - Pin nbconvert version in CI job
[medium · 88% confidence]in.github/workflows/compliance.yaml:56(details inline) - Pin version of nbconvert dependency
[medium · 88% confidence]in.pre-commit-config.yaml:57(details inline) - Use consistent variable quoting in shell script
[medium · 87% confidence]in.github/workflows/compliance.yaml:58(details inline) - Unquoted variable expansion may break notebook output stripping
[medium · 87% confidence]in.github/workflows/compliance.yaml:57(details inline) - Unquoted variable expansion in git diff may miss notebook changes
[medium · 87% confidence]in.github/workflows/compliance.yaml:58(details inline) - no-merge-commits pre-commit hook does not detect existing merge commits
[medium · 84% confidence]in.pre-commit-config.yaml:65
The hook namedno-merge-commitschecks only for the presence of.git/MERGE_HEAD, which indicates an ongoing merge operation. It does not detect merge commits that already exist in the branch history, so the pre‑commit stage can allow a commit that introduces a merge commit, violating the linear‑history policy documented in the guidelines.
Fix: Replace the current entry with a command that scans the branch for merge commits, e.g. usinggit rev-list --mergesbetween the upstream base and HEAD, and run the hook in thepre-pushstage (orpre-commitwith a proper range) to enforce the rule before pushing. Example entry:bash -c 'BASE=$(git merge-base @{upstream} HEAD); if git rev-list --merges "$BASE"..HEAD | grep .; then echo "Error: merge commits are not allowed in DPsim. Please rebase onto the target branch."; exit 1; fi'. - no-merge-commits hook does not detect existing merge commits
[medium · 84% confidence]in.pre-commit-config.yaml:65
The local pre‑commit hook "no-merge-commits" only checks for the presence of a .git/MERGE_HEAD file, which indicates an in‑progress merge. It does not detect merge commits already present in the branch history, so a user can commit a branch that already contains merge commits without being blocked, violating the Linear History policy (Development/guidelines). The CI workflow correctly detects such commits, but the pre‑commit hook fails to enforce the rule locally.
Fix: Replace the hook's entry with a command that scans the branch for merge commits, e.g. usinggit log --mergesbetween the branch base and HEAD, similar to the CI job. This will block commits that introduce merge commits. - Misleading hook name and description for merge‑commit guard
[medium · 82% confidence]in.pre-commit-config.yaml:65
The local pre‑commit hook is named and described as "no merge commits (use rebase)" and its id isno-merge-commits. However the implementation only checks for the presence of.git/MERGE_HEAD, which indicates an in‑progress merge operation, not whether the branch history contains merge commits. Consequently the hook does not enforce the documented linear‑history policy and gives a false sense of safety.
Fix: Replace the MERGE_HEAD check with a detection of merge commits in the branch range, e.g. usinggit log --mergesas done in the CI workflow, or rename the hook to reflect its actual behaviour.
🔵 Optional / low-confidence (3)
- Notebook‑output pre‑commit hook may break CI on existing notebooks
[high · 35% confidence · unconfirmed]in.pre-commit-config.yaml:59 - Clarify 'upstream/master' to 'upstream/main' in no-merge-commits hook
[medium · 30% confidence · unconfirmed]in.pre-commit-config.yaml:70 - Replace 'master' with 'main' in compliance workflow
[medium · 25% confidence · unconfirmed]in.github/workflows/compliance.yaml:31
Claim vs. implementation
- Claimed: Add CONTRIBUTING.md plus CI and pre-commit compliance checks for merge commits and notebook outputs, with docs updated to match.
- Done: Adds a root CONTRIBUTING.md, updates contribution/development docs, and adds CI/pre-commit checks for merge commits and notebook output stripping.
- Difference: The notebook CI check is not scoped to notebooks touched by the PR as claimed; it clears outputs on the touched notebooks and then fails if any diff remains, which can also be affected by the strip operation itself.
How this review was produced
13 specialized finder passes raised 38 findings over the diff and the full changed sources. After de-duplication, 36 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 22 refuted as unsupported, 14 kept (3 tentative).
Refuted by verification:
- Use conventional commit style consistently in example (CONTRIBUTING.md):
fix: correct node voltage initialization in DiakopticsSolveris a valid conventional-commit example - Use DOUBLE_EPSILON for epsilon comparisons in notebook output check (.github/workflows/compliance.yaml): the workflow checks textual git diffs after output stripping, not numeric values requiring
DOUBLE_EPSILON - Specify Python version for nbconvert in pre-commit hook (.pre-commit-config.yaml): the hook is a pre-commit python-language hook using its managed environment, and no file evidence establishes a required pinned interpreter
- Clarify pre-commit hook installation scope (CONTRIBUTING.md): the text only says
pre-commit installactivates checks and does not claim all hooks run on the entire repository - Local pre-commit hook runs on entire repo and can block PRs (.pre-commit-config.yaml): pre-commit file filtering does not run on the entire repository by default; it receives matching staged files unless invoked with --all-files
- Compliance job may run on unrelated notebooks (.github/workflows/compliance.yaml):
NOTEBOOKSis populated bygit diff ... "$BASE" "$HEAD" -- '*.ipynb', so the job is scoped to PR-touched notebooks - Unconditional pip install in CI may fail if nbconvert is unavailable (.github/workflows/compliance.yaml): the script exits before
pip install nbconvertwhen$NOTEBOOKSis empty - Pre-commit hook entry uses deprecated Python module invocation (.pre-commit-config.yaml):
python -m nbconvertis a normal module invocation inside a python pre-commit environment, not shown here to be deprecated - Use consistent commit message style in example (CONTRIBUTING.md): the example is a bug-fix commit, so using the
fix:conventional type is consistent - Use consistent variable quoting in shell script (.github/workflows/compliance.yaml): line 52 does not expand
$NOTEBOOKS; it assigns it using quoted$BASEand$HEAD - Fix conventional commit example in CONTRIBUTING.md (CONTRIBUTING.md): node voltage initialization can reasonably be a bug fix and the example follows conventional commit syntax
- Consistent branch list formatting in compliance workflow (.github/workflows/compliance.yaml):
branches: [master, main]contains two distinct branch names, not duplicate identical entries - Clarify logging rule in contribution checklist (CONTRIBUTING.md): the checklist accurately states the core rule to avoid
std::coutand useSPDLOG_LOGGER_*macros - Clarify spdlog macro usage in logging section (docs/hugo/content/en/docs/Development/guidelines.md): the cited logging text is outside the changed lines for this file
- Fix example commit command to use conventional commit style (CONTRIBUTING.md): the existing command already matches the conventional-commit form suggested by the finding
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.
No-merge-commits pre-commit hook does not detect existing merge commits
severity: high · confidence: 86%
The hook checks only for the presence of .git/MERGE_HEAD, which is set only during an ongoing merge operation. It fails to detect merge commits that are already part of the branch history, allowing branches with merge commits to pass the pre‑commit stage while the CI workflow correctly rejects them. This inconsistency defeats the intended linear‑history enforcement.
Suggested fix: Replace the current entry with a command that runs git log --merges over the range of commits being added, similar to the CI workflow. For example, use git log --merges "$(git merge-base HEAD @{upstream})"..HEAD --oneline and abort if any output is found.
| bash -c '[ ! -f .git/MERGE_HEAD ] || { | |
| entry: >- | |
| bash -c 'MERGES=$(git log --merges "$(git merge-base HEAD @{upstream})"..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 hook only tests [ ! -f .git/MERGE_HEAD ], which detects an in-progress merge rather than merge commits already in branch history
stage: numerics
| name: no merge commits (use rebase) | ||
| language: system | ||
| entry: >- | ||
| bash -c '[ ! -f .git/MERGE_HEAD ] || { |
There was a problem hiding this comment.
Ineffective no‑merge‑commits pre‑commit hook
severity: high · confidence: 86%
The no-merge-commits hook only checks for the presence of .git/MERGE_HEAD, which exists only during an ongoing merge operation. It does not detect existing merge commits in the branch history, allowing a developer to commit merge commits that the hook will miss. The CI workflow correctly detects them, but the pre‑commit hook fails to enforce the linear‑history policy locally.
Suggested fix: Replace the hook command with a git query that scans the current branch for merge commits relative to the upstream default branch (e.g. origin/master). A simple implementation is:
bash -c 'BASE=$(git merge-base HEAD origin/master); MERGES=$(git log --merges "$BASE"..HEAD --oneline); if [ -n "$MERGES" ]; then echo "Error: merge commits are not allowed in DPsim."; echo "$MERGES"; exit 1; fi'
| bash -c '[ ! -f .git/MERGE_HEAD ] || { | |
| - id: no-merge-commits | |
| name: no merge commits (use rebase) | |
| language: system | |
| entry: >- | |
| bash -c 'BASE=$(git merge-base HEAD origin/master); MERGES=$(git log --merges "$BASE"..HEAD --oneline); if [ -n "$MERGES" ]; then echo ""; echo "Error: merge commits are not allowed in DPsim."; echo "$MERGES"; echo ""; 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' | |
| stages: [pre-commit] | |
| always_run: true | |
| pass_filenames: false |
Checked against the source: the no-merge hook relies solely on .git/MERGE_HEAD, which exists only during an active merge operation
stage: build-deps
| - id: clear-notebook-outputs | ||
| name: clear notebook outputs | ||
| language: python | ||
| additional_dependencies: ["nbconvert"] |
There was a problem hiding this comment.
Pin nbconvert version to avoid future breakage
severity: medium · confidence: 88%
The local pre-commit hook for clearing notebook outputs uses nbconvert without a version pin. Upstream releases can change CLI behavior or defaults, causing the hook to break or behave unexpectedly. Pinning the version ensures reproducibility and aligns with DPsim's policy of pinning tool versions in pre-commit hooks.
| additional_dependencies: ["nbconvert"] | |
| entry: python -m nbconvert==7.16.6 --ClearOutputPreprocessor.enabled=True --inplace |
Checked against the source: additional_dependencies: ["nbconvert"] installs nbconvert without a version constraint
stage: build-deps
| if [ -z "$NOTEBOOKS" ]; then | ||
| exit 0 | ||
| fi | ||
| pip install nbconvert |
There was a problem hiding this comment.
Pin nbconvert version in CI job
severity: medium · confidence: 88%
The CI job installs nbconvert without a version pin. Upstream releases can change behavior or defaults, which may break the job or cause unexpected output stripping. Pinning the version ensures the job remains stable across time.
| pip install nbconvert | |
| pip install nbconvert==7.16.6 |
Checked against the source: the workflow installs nbconvert with pip install nbconvert and no version pin
stage: build-deps
| - id: clear-notebook-outputs | ||
| name: clear notebook outputs | ||
| language: python | ||
| additional_dependencies: ["nbconvert"] |
There was a problem hiding this comment.
Pin version of nbconvert dependency
severity: medium · confidence: 88%
The pre-commit hook clear-notebook-outputs installs the Python package nbconvert without a version constraint. This can lead to nondeterministic CI runs and potential incompatibilities if a future nbconvert release changes its CLI or behavior, breaking the notebook‑output stripping step.
Suggested fix: Add an explicit version spec to the additional_dependencies list, e.g. nbconvert==7.14.0, and keep it updated in lock files.
| additional_dependencies: ["nbconvert"] | |
| - id: clear-notebook-outputs | |
| name: clear notebook outputs | |
| language: python | |
| additional_dependencies: ["nbconvert==7.14.0"] | |
| entry: python -m nbconvert --ClearOutputPreprocessor.enabled=True --inplace | |
| files: \.ipynb$ |
Checked against the source: the nbconvert dependency is listed as bare nbconvert with no pinned version
stage: build-deps
| fi | ||
| pip install nbconvert | ||
| python -m nbconvert --ClearOutputPreprocessor.enabled=True --inplace $NOTEBOOKS | ||
| if ! git diff --quiet -- $NOTEBOOKS; then |
There was a problem hiding this comment.
Use consistent variable quoting in shell script
severity: medium · confidence: 87%
The script uses unquoted $NOTEBOOKS in the git diff command (line 58), which can cause issues if the variable contains spaces or special characters. Shell variables should be quoted to avoid word splitting and globbing.
Suggested fix: Quote the $NOTEBOOKS variable in the git diff command to ensure robustness.
| if ! git diff --quiet -- $NOTEBOOKS; then | |
| if ! git diff --quiet -- "$NOTEBOOKS"; then |
Checked against the source: git diff --quiet -- $NOTEBOOKS expands the notebook list unquoted
stage: cpp-design
| exit 0 | ||
| fi | ||
| pip install nbconvert | ||
| python -m nbconvert --ClearOutputPreprocessor.enabled=True --inplace $NOTEBOOKS |
There was a problem hiding this comment.
Unquoted variable expansion may break notebook output stripping
severity: medium · confidence: 87%
The script expands $NOTEBOOKS without quoting. If any notebook filename contains spaces or special characters, the command python -m nbconvert ... $NOTEBOOKS will split the list incorrectly, causing the notebook‑output stripping step to fail or process only a subset of files, potentially letting notebooks with saved outputs pass the check.
Suggested fix: Quote the variable expansion to preserve the list as a single argument list. Use "$NOTEBOOKS" in both the nbconvert command and the subsequent git diff check.
| python -m nbconvert --ClearOutputPreprocessor.enabled=True --inplace $NOTEBOOKS | |
| python -m nbconvert --ClearOutputPreprocessor.enabled=True --inplace "$NOTEBOOKS" |
Checked against the source: python -m nbconvert ... $NOTEBOOKS expands the notebook path list unquoted
stage: numerics
| fi | ||
| pip install nbconvert | ||
| python -m nbconvert --ClearOutputPreprocessor.enabled=True --inplace $NOTEBOOKS | ||
| if ! git diff --quiet -- $NOTEBOOKS; then |
There was a problem hiding this comment.
Unquoted variable expansion in git diff may miss notebook changes
severity: medium · confidence: 87%
The git diff --quiet -- $NOTEBOOKS command also expands $NOTEBOOKS without quoting, leading to the same word‑splitting issue as above. This can cause the diff check to ignore notebooks with spaces in their paths, allowing them to retain outputs unnoticed.
Suggested fix: Quote the variable expansion similarly to ensure all notebook paths are correctly passed to git diff.
| if ! git diff --quiet -- $NOTEBOOKS; then | |
| if ! git diff --quiet -- "$NOTEBOOKS"; then |
Checked against the source: git diff --quiet -- $NOTEBOOKS expands notebook paths unquoted
stage: numerics
There was a problem hiding this comment.
DPsim LLM review
Found 1 high, 7 medium (7 anchored to lines below).
🔴 Critical & high
- no-merge-commits pre-commit hook does not detect existing merge commits
[high · 84% confidence]in.pre-commit-config.yaml:65(details inline)
🟡 Suggestions
- pre-commit no-merge-commits hook does not detect existing merge commits
[medium · 84% confidence]in.pre-commit-config.yaml:65(details inline) - Missing nbconvert version pin in CI job
[medium · 82% confidence]in.github/workflows/compliance.yaml:56(details inline) - Pin nbconvert version in pre-commit hook
[medium · 82% confidence]in.pre-commit-config.yaml:57(details inline) - Pin nbconvert version in CI to ensure reproducibility
[medium · 82% confidence]in.github/workflows/compliance.yaml:56(details inline) - Pin nbconvert version in pre-commit hook to match CI
[medium · 82% confidence]in.pre-commit-config.yaml:57(details inline) - Misleading merge‑commit detection comment
[medium · 80% confidence]in.pre-commit-config.yaml:62(details inline)
🔵 Optional / low-confidence (1)
- Use conventional commit style consistently in example
[medium · 20% confidence · unconfirmed]inCONTRIBUTING.md:18
How this review was produced
13 specialized finder passes raised 28 findings over the diff and the full changed sources. After de-duplication, 26 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, 8 kept (1 tentative).
Refuted by verification:
- Pre-commit hook for notebook outputs should run only on staged files (.pre-commit-config.yaml): the hook does not disable filename passing, so pre-commit applies the entry to matched staged filenames rather than all notebooks
- Missing error handling for nbconvert installation failure (.github/workflows/compliance.yaml): a failing
pip install nbconvertcommand in a GitHub Actions run step causes the step to fail before the following command - Use of --diff-filter=d is fragile for detecting new notebooks (.github/workflows/compliance.yaml):
--diff-filter=dintentionally excludes deleted files while still considering non-deleted changed notebooks - Use consistent commit message style in example (CONTRIBUTING.md):
fix:is explicitly listed as an accepted conventional commit type on line 15 and is valid for an example - Incorrect diff-filter for notebook outputs check (.github/workflows/compliance.yaml): deleted notebooks cannot retain committed outputs in the resulting PR tree and including them would pass nonexistent paths to nbconvert
- Typo in hook name (.pre-commit-config.yaml): pre-commit hook names are human-readable labels and line 55's spaced name is not a functional defect
- Fix example commit command to use conventional commit style (CONTRIBUTING.md): line 15 lists multiple conventional commit types (
feat:,fix:,docs:), so the example does not imply onlyfix:is supported - Update rebase command to match CONTRIBUTING.md (docs/hugo/content/en/docs/Contribution guidelines/index.md): the quick-start command uses
git rebase upstream/master, the same command shown in CONTRIBUTING.md - Clarify rebase command in Development Guidelines (docs/hugo/content/en/docs/Development/guidelines.md): the guidelines use
git rebase upstream/master, matching CONTRIBUTING.md rather than conflicting with it - Clarify rebase command in pre-commit hook message (.pre-commit-config.yaml): the message uses
git rebase upstream/master, matching the stated command rather than differing from it - Incorrect logging macro reference in documentation (docs/hugo/content/en/docs/Development/guidelines.md): line 25 references
SPDLOG_LOGGER_INFO, notSPDLOG_INFO - Add missing link to Development Guidelines in CONTRIBUTING.md (CONTRIBUTING.md): CONTRIBUTING.md already links to the Development Guidelines on line 8, so the link is not missing
- Update Development Guidelines link to use Hugo shortcode (docs/hugo/content/en/docs/Contribution guidelines/index.md): line 13 already links to Development Guidelines using the Hugo
relrefshortcode - Add missing SPDX header to Markdown file (CONTRIBUTING.md): the new Markdown file contains SPDX copyright and license identifiers in an HTML comment header on lines 1-4
- Add missing SPDX header to Markdown file (docs/hugo/content/en/docs/Contribution guidelines/index.md): the diff shows this documentation page was modified rather than newly added, so a pre-existing missing header is out of scope
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.
no-merge-commits pre-commit hook does not detect existing merge commits
severity: high · confidence: 84%
The hook intended to enforce a linear git history checks only for the presence of .git/MERGE_HEAD, which indicates an ongoing merge operation. It does not examine the commit history for merge commits that already exist in the branch. Consequently, branches containing merge commits will pass the pre‑commit check, violating the documented rule and allowing non‑linear history to be introduced.
Suggested fix: Replace the current check with a git log query similar to the CI job, e.g., git log --merges $(git merge-base HEAD @{upstream})..HEAD --oneline. Fail if any output is found. Ensure the hook runs on the staged changes (or the whole branch) and exits with a non‑zero status when merge commits are detected.
| bash -c '[ ! -f .git/MERGE_HEAD ] || { | |
| entry: >- | |
| bash -c 'MERGES=$(git log --merges $(git merge-base HEAD @{upstream})..HEAD --oneline); if [ -n "$MERGES" ]; then echo "Error: merge commits are not allowed in DPsim."; echo "$MERGES"; echo "Please rebase your branch onto the target branch instead:"; echo " git rebase upstream/master"; echo " git push --force-with-lease"; exit 1; fi' |
Checked against the source: the no-merge hook checks only .git/MERGE_HEAD, which detects an in-progress merge rather than existing merge commits
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.
pre-commit no-merge-commits hook does not detect existing merge commits
severity: medium · confidence: 84%
The hook checks only for the presence of .git/MERGE_HEAD, which indicates an ongoing merge operation, but does not detect merge commits already present in the branch history. This fails to enforce the linear history policy documented in the guidelines, allowing branches with merge commits to pass the pre-commit check.
Suggested fix: Replace the current entry with a script that scans the commit history for merge commits, e.g., using git rev-list --merges --first-parent HEAD and failing if any output is produced. Ensure the hook works without PR context, detecting merges in the current branch.
| bash -c '[ ! -f .git/MERGE_HEAD ] || { | |
| entry: >- | |
| bash -c 'if git rev-list --merges --first-parent HEAD | grep -q .; then | |
| echo "\nError: merge commits are not allowed in DPsim.\nPlease rebase your branch onto the target branch instead:\n\n git rebase upstream/master\n git push --force-with-lease\n"; exit 1; fi' |
Checked against the source: the hook only tests [ ! -f .git/MERGE_HEAD ] and does not inspect branch history for merge commits
stage: domain-modeling
| if [ -z "$NOTEBOOKS" ]; then | ||
| exit 0 | ||
| fi | ||
| pip install nbconvert |
There was a problem hiding this comment.
Missing nbconvert version pin in CI job
severity: medium · confidence: 82%
The CI job installs nbconvert without a version pin. This can lead to non-reproducible builds if nbconvert introduces breaking changes in a minor release. The job should pin the version to a known-good release (e.g., 'pip install nbconvert==7.16.4').
Suggested fix: Pin the nbconvert version in the CI job to ensure reproducible builds. Update line 56 to: pip install nbconvert==7.16.4
Checked against the source: the workflow installs nbconvert with pip install nbconvert and no version constraint
stage: io-robustness
| - 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
severity: medium · confidence: 82%
The pre-commit hook entry for clearing notebook outputs installs nbconvert without a version pin. This can cause the hook to break if nbconvert changes its CLI or behavior. The hook should pin the version to match the CI job (e.g., 'nbconvert==7.16.4').
Suggested fix: Pin the nbconvert version in the pre-commit hook to ensure consistency with CI. Update the entry to: additional_dependencies: ["nbconvert==7.16.4"]
Checked against the source: additional_dependencies: ["nbconvert"] installs nbconvert without a version pin
stage: io-robustness
| if [ -z "$NOTEBOOKS" ]; then | ||
| exit 0 | ||
| fi | ||
| pip install nbconvert |
There was a problem hiding this comment.
Pin nbconvert version in CI to ensure reproducibility
severity: medium · confidence: 82%
The CI step installs nbconvert without a pinned version (pip install nbconvert). This can lead to non-reproducible builds if the latest nbconvert changes its behavior or dependencies, causing the notebook output stripping to fail or behave differently across CI runs.
Suggested fix: Pin the nbconvert version to a known-good release (e.g., pip install nbconvert==7.16.6) to ensure the CI job remains stable and reproducible. Update the corresponding pre-commit hook entry in .pre-commit-config.yaml to use the same pinned version.
Checked against the source: line 56 uses an unpinned pip install nbconvert, making the CI tool version non-reproducible
stage: build-deps
| - 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 match CI
severity: medium · confidence: 82%
The pre-commit hook entry for clearing notebook outputs installs nbconvert without a pinned version (additional_dependencies: ["nbconvert"]). This risks divergence between CI and local pre-commit behavior, potentially causing the hook to succeed locally while CI fails, or vice versa.
Suggested fix: Pin the nbconvert version in the pre-commit hook to the same version used in CI (e.g., additional_dependencies: ["nbconvert==7.16.6"]).
Checked against the source: additional_dependencies: ["nbconvert"] is unpinned, so local hook behavior can track latest nbconvert
stage: build-deps
| files: \.ipynb$ | ||
|
|
||
| - id: no-merge-commits | ||
| name: no merge commits (use rebase) |
There was a problem hiding this comment.
Misleading merge‑commit detection comment
severity: medium · confidence: 80%
The hook "no-merge-commits" advertises that it blocks merge commits ("no merge commits (use rebase)"), but the implementation only checks for the presence of .git/MERGE_HEAD, which indicates an in‑progress merge operation, not existing merge commits in the branch history. This discrepancy can mislead contributors into believing the hook enforces the linear‑history policy, while it actually does not catch already‑created merge commits, potentially allowing prohibited history to pass the pre‑commit check.
Suggested fix: Replace the MERGE_HEAD check with a git log query that detects merge commits in the current branch range, e.g. git rev-parse --verify HEAD && git log --merges --oneline $(git merge-base HEAD @{u})..HEAD and fail if any output is found. Alternatively, document that the hook only prevents interactive merges and rely on the CI workflow for full detection.
Checked against the source: the hook is named no merge commits but its implementation only checks .git/MERGE_HEAD
stage: naming-docs
pipeacosta
left a comment
There was a problem hiding this comment.
Thanks for your PR @leonardocarreras. The guidelines are useful and coherent. I just added a grammar check comment. In addition, a couple of suggestions from the LLM might make sense, e.g. the one that has to do with the nbconvert version and the merge commit detection.
|
|
||
| # Creating New Releases (info for maintainers) | ||
|
|
||
| DPsim currently uses to [Semantic Versioning](https://semver.org/). The periodic creation of |
There was a problem hiding this comment.
Please correct the grammar of the statement: DPsim currently uses to [Semantic Versioning](https://semver.org/)
…#605) Mechanical pass, no notebook content changes: outputs cleared and missing cell ids backfilled across 113 notebooks. Split out of #509 so the contribution policy there stays reviewable. The pre-commit hook and CI guard that keep this true land with #509 and scope themselves to notebooks a pull request touches, so this can merge in either order. Regenerated rather than rebased if it goes stale.
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: The diff does not implement the claimed merge-commit compliance check; the workflow only checks notebook outputs, and the notebook hook is repo-wide rather than scoped to the pull request range.
TL;DR: One real blocker remains: the new pre-commit notebook-output hook is repo-wide and will keep CI red until the existing notebooks with saved outputs are cleaned or the hook is narrowed to PR-changed files; the rest of the compliance logic is scoped correctly per the PR intent. No other distinct findings were provided.
Found 1 critical (0 anchored to lines below).
🔵 Optional / low-confidence (1)
- Notebook clearing hook runs on all notebooks causing CI failures
[critical · 35% confidence · unconfirmed]in.pre-commit-config.yaml:59
Claim vs. implementation
- Claimed: Add CONTRIBUTING.md plus CI/pre-commit compliance checks for linear history and notebook output stripping, with docs updated to match.
- Done: Adds CONTRIBUTING.md, updates contribution/development docs, adds a pull-request CI workflow that rejects changed notebooks with saved outputs, and adds a pre-commit hook to clear notebook outputs.
- Difference: The diff does not implement the claimed merge-commit compliance check; the workflow only checks notebook outputs, and the notebook hook is repo-wide rather than scoped to the pull request range.
How this review was produced
13 specialized finder passes raised 27 findings over the diff and the full changed sources. After de-duplication, 27 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 25 refuted as unsupported, 2 kept (2 tentative).
Refuted by verification:
- Use conventional commit style consistently in example (CONTRIBUTING.md): The file already states to commit using the conventional commit style with examples like
feat:,fix:,docs:. - Git diff command may fail on Windows due to path separator (.github/workflows/compliance.yaml): The workflow runs on ubuntu-latest with bash and uses git diff paths directly; no Windows runner or path-normalization issue is present in this file.
- Pre-commit hook entry may not handle spaces in notebook paths (.pre-commit-config.yaml): The hook already targets notebook filenames with
files: \.ipynb$; no evidence in this file that spaces or special characters are mishandled. - Use consistent shell quoting in code blocks (CONTRIBUTING.md): The quick-start and rebase examples are both shell code fences (
```shell), so there is no quoting/style mismatch. - Use consistent shell quoting in code blocks (docs/hugo/content/en/docs/Contribution guidelines/index.md): The file consistently uses inline code for the single command and a fenced bash block for the multi-line rebase example, which is normal Markdown style.
- Use NUL-delimited git diff carefully (.github/workflows/compliance.yaml): The script already uses bash intentionally (
shell: bash) and the NUL-delimitedmapfile -d ''pattern is valid here. - Fix inconsistent shell code block style (CONTRIBUTING.md): The commit example is inside a shell code block, not backticks-only inline code, so the alleged visual inconsistency is absent.
- Fix inconsistent shell code block style (CONTRIBUTING.md): The rebase example is correctly introduced by the numbered step ending with a colon, followed by a shell code block.
- Fix inconsistent shell code block style (docs/hugo/content/en/docs/Contribution guidelines/index.md): The preceding list item already ends with a colon before the fenced code block, so the claimed mismatch is not present.
- Fix typo in code example (docs/hugo/content/en/docs/Development/guidelines.md): The documented command intentionally uses
git rebase upstream/masterin the new Linear History section. - Use NUL-safe git diff to avoid path-list injection (.github/workflows/compliance.yaml): The later
git diff --quiet -- "${NOTEBOOKS[@]}"andgit diff --name-only -- "${NOTEBOOKS[@]}"operate on quoted array elements, so there is no argument-splitting bug. - Pin nbconvert version to avoid CI breakage (.github/workflows/compliance.yaml):
pip install nbconvertis present exactly as claimed, but version pinning is a policy preference rather than a defect in the file itself. - Pin nbconvert version in pre-commit hook (.pre-commit-config.yaml): The hook already pins its dependency via
additional_dependencies: ["nbconvert"], which is the only version control present in this file. - Pin nbconvert version in CI to avoid supply-chain drift (.github/workflows/compliance.yaml): The workflow intentionally installs nbconvert without a pin; the file does not show a correctness bug from that alone.
- Update Development Guidelines URL to relative path (CONTRIBUTING.md): The Development Guidelines link is already a relative-to-site absolute URL in the document and the file intentionally references the hosted docs.
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.
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
745b8cb to
be50060
Compare
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de> docs: drop the merge-commit checks and harden the notebook compliance job Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
… job Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
…tting Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
be50060 to
0e25225
Compare
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: matches the description.
Found 2 medium (0 anchored to lines below).
🔵 Optional / low-confidence (2)
- Pin nbconvert version in pre-commit hook to avoid drift
[medium · 35% confidence · unconfirmed]in.pre-commit-config.yaml:57 - Undocumented third‑party dependency nbconvert
[medium · 35% confidence · unconfirmed]in.pre-commit-config.yaml:57
Claim vs. implementation
- Claimed: Add a root CONTRIBUTING.md, update contribution/development docs, and add CI/pre-commit checks that reject notebooks with saved outputs.
- Done: Adds CONTRIBUTING.md, updates the two docs pages to match it, adds a pull_request workflow that scans changed .ipynb files for outputs/execution counts, adds a pre-commit hook to clear notebook outputs, and normalizes one notebook’s formatting/metadata.
- Difference: none
How this review was produced
13 specialized finder passes raised 36 findings over the diff and the full changed sources. After de-duplication, 36 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 34 refuted as unsupported, 2 kept (2 tentative).
Refuted by verification:
- Clarify linear-history policy wording (CONTRIBUTING.md): The file already says "never merge the target branch into your feature branch; rebase instead," which is the linear-history rule being requested.
- Attribute usage: prefer typed members over string attribute lookups (CONTRIBUTING.md): Line 38 is about notebook outputs, not attribute access, so the claimed recommendation vs hard rule issue is not present in this file.
- Document linear history policy precisely (CONTRIBUTING.md): The policy is explicit on line 33: "never merge the target branch into your feature branch; rebase instead," leaving no ambiguity in the text.
- Use NUL-delimited diff to avoid path issues (.github/workflows/compliance.yaml): The workflow explicitly runs under
shell: bash, andmapfile -d ''is used with NUL-delimitedgit diff -zoutput on line 32. - Improve error message formatting (.github/workflows/compliance.yaml): The error output is just
print(f" {path}"); the leading spaces are intentional formatting, not a defect. - Add missing step to keep CONTRIBUTING.md in sync with guidelines (CONTRIBUTING.md): The quick start already includes step 6: "Open a pull request against
sogno-platform/dpsim:master." - Update quick start to match CONTRIBUTING.md (docs/hugo/content/en/docs/Contribution guidelines/index.md): The quick start already includes step 5: 'Open a pull request from your fork.'
- Inconsistent phrasing for pre-commit activation (docs/hugo/content/en/docs/Contribution guidelines/index.md): The wording is present in the changed line and 'etc.' is a style preference, not a defect in the file
- Add SPDX header to CONTRIBUTING.md (CONTRIBUTING.md): The file already has an SPDX header at the top, including SPDX-FileCopyrightText and SPDX-License-Identifier.
- Update page title to match site navigation (docs/hugo/content/en/docs/Contribution guidelines/index.md): Front matter title and linkTitle both already use 'Contribution Guidelines', and no SPDX header is required for this existing markdown file
- Remove redundant 'new file' comment in SPDX header (.github/workflows/compliance.yaml): The SPDX header is present and does not contain any redundant 'new file' text on line 1.
- Add missing SPDX header to CONTRIBUTING.md (CONTRIBUTING.md): The SPDX header is present in HTML comment syntax on lines 1-4, so there is no missing-header defect.
- Add missing SPDX header to contribution guidelines page (docs/hugo/content/en/docs/Contribution guidelines/index.md): This is not a new file; the page already has front matter and the SPDX-header rule applies to new source/script/workflow files, not this existing doc
- Workflow uses untrusted PR event metadata for privileged action (.github/workflows/compliance.yaml): The workflow reads
github.event.pull_request.base.shaandhead.shaonly to diff the PR's changed notebooks; this is the standard pull_request context for a read-only compliance check. - Rename documentation directory to avoid spaces (docs/hugo/content/en/docs/Contribution guidelines/index.md): The directory name with a space is part of the existing path and the file content cannot establish a naming-convention violation
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.
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: matches the description.
No issues surfaced by the automated passes.
Claim vs. implementation
- Claimed: Add CONTRIBUTING.md, update contribution/development docs, and add CI/pre-commit checks to reject notebooks with saved outputs.
- Done: Adds a root CONTRIBUTING.md, updates the two docs pages to match it, adds a pull_request workflow that fails changed notebooks with outputs/execution counts, adds a local pre-commit hook to clear notebook outputs, and normalizes one notebook’s saved metadata/outputs.
- Difference: none
How this review was produced
13 specialized finder passes raised 39 findings over the diff and the full changed sources. After de-duplication, 39 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 39 refuted as unsupported, 0 kept (0 tentative).
Refuted by verification:
- Clarify linear history policy wording (CONTRIBUTING.md): The line already says "never merge the target branch into your feature branch; rebase instead," which clearly targets merge commits in this workflow context.
- Use consistent command-line formatting for commit examples (CONTRIBUTING.md): The commit example is already shown in a shell code block with
git commit -s -m .... - Clarify rebase workflow with explicit upstream reference (CONTRIBUTING.md): The rebase example explicitly references
upstreamandupstream/master. - Add SPDX header to CONTRIBUTING.md (CONTRIBUTING.md): The file already has an SPDX copyright and license header in an HTML comment at the top.
- Clarify notebook output policy in CONTRIBUTING.md (CONTRIBUTING.md): The notebook-output policy already states that pre-commit and CI both enforce it.
- Add missing SPDX header to CONTRIBUTING.md (CONTRIBUTING.md): The Markdown file already uses the documented HTML-comment SPDX header syntax with both copyright and license lines.
- Incorrect DP-to-EMT reconstruction scaling in envelope conversion (examples/Notebooks/Components/DP_Ph3_AvVoltSourceInverterStateSpace.ipynb): reconstruct_to_emt multiplies by a scale argument and callers pass dpsimpy.RMS3PH_TO_PEAK1PH
- Clarify scaling convention in notebook comment (examples/Notebooks/Components/DP_Ph3_AvVoltSourceInverterStateSpace.ipynb): the opening comment explicitly names dpsimpy.RMS3PH_TO_PEAK1PH
- Replace ad-hoc zero check with DOUBLE_EPSILON (examples/Notebooks/Components/DP_Ph3_AvVoltSourceInverterStateSpace.ipynb): the cited literal is on line 167, which is outside the changed notebook lines
- Use DOUBLE_EPSILON for discriminant sign check (examples/Notebooks/Components/DP_Ph3_AvVoltSourceInverterStateSpace.ipynb): the discriminant check is on line 178, outside the changed notebook lines
- Use DOUBLE_EPSILON for sqrt discriminant guard (examples/Notebooks/Components/DP_Ph3_AvVoltSourceInverterStateSpace.ipynb): sqrt is preceded by a negative-discriminant guard and the code is outside the changed lines
- Replace ad-hoc tolerance with DOUBLE_EPSILON (examples/Notebooks/Components/DP_Ph3_AvVoltSourceInverterStateSpace.ipynb): the 1e-2 assertion is on line 661, outside the changed notebook lines
- Replace ad-hoc tolerance with DOUBLE_EPSILON (examples/Notebooks/Components/DP_Ph3_AvVoltSourceInverterStateSpace.ipynb): the healthy-phase 1e-2 assertions are on lines 666-667, outside the changed notebook lines
- Use DOUBLE_EPSILON for near-zero assertion (examples/Notebooks/Components/DP_Ph3_AvVoltSourceInverterStateSpace.ipynb): the cited 1e-3 tolerance is on line 732, outside the changed notebook lines
- Clarify linear history policy wording (docs/hugo/content/en/docs/Development/guidelines.md): the section title says no merge commits and the instruction specifically forbids merging the target branch into the feature branch
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.
|
| print("Error: the following notebooks have saved outputs committed:") | ||
| for path in offenders: | ||
| print(f" {path}") | ||
| print("") |
| print("Error: the following notebooks have saved outputs committed:") | ||
| for path in offenders: | ||
| print(f" {path}") | ||
| print("") |
georgii-tishenin
left a comment
There was a problem hiding this comment.
Thanks @leonardocarreras. Good stuff! I think this should eventually make contributing and reviewing easier. From my point of view looks fine.
But we can see if @pipeacosta and @stv0g have further comments.



Adds CONTRIBUTING.md at the repository root, where GitHub surfaces it when opening a pull request, and brings the Contribution and Development Guidelines pages in line with it.
A compliance workflow fails a pull request that changes a notebook still carrying saved outputs, scoped to the range the pull request touches and also available as a pre-commit hook. Linear history stays documented as a policy, since merge commits are a matter for the repository settings.
This is the base of a stack: #607 adds conventional commits on top, #608 adds release-please.