Skip to content

Add nightly supply chain review - #171

Open
ajnelson-nist wants to merge 5 commits into
developfrom
add_nightly_supply_chain_review
Open

Add nightly supply chain review#171
ajnelson-nist wants to merge 5 commits into
developfrom
add_nightly_supply_chain_review

Conversation

@ajnelson-nist

Copy link
Copy Markdown
Member

This PR adds nightly supply chain review against the develop branch.

This PR is similar to case-prov PR 109 The check-supply-chain-pre-commit target also is updated to only fail CI if the pre-commit update would cause any files to be re-formatted. (The "Relax pre-commit update enforcement" patch was drawn from the CDO-Shapes-Example repository.)

Disclaimer

Participation by NIST in the creation of the documentation of mentioned software is not intended to imply a recommendation or endorsement by the National Institute of Standards and Technology, nor is it intended to imply that any specific software is necessarily the best available for the purpose.

No effects were observed on Make-managed files.

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
@ajnelson-nist ajnelson-nist added this to the 0.17.0 milestone Nov 15, 2024
@ajnelson-nist

Copy link
Copy Markdown
Member Author

This PR is ready for review and merge.

@ajnelson-nist
ajnelson-nist marked this pull request as ready for review November 15, 2024 17:09
@ajnelson-nist
ajnelson-nist requested a review from a team as a code owner November 15, 2024 17:09
@ajnelson-nist

Copy link
Copy Markdown
Member Author

Postponing to 0.18.0. There's also a line that should go into the PR to have this target develop, not the primary branch.

@ajnelson-nist
ajnelson-nist marked this pull request as draft June 17, 2025 13:24
@ajnelson-nist ajnelson-nist modified the milestones: 0.17.0, 0.18.0 Jun 17, 2025
@ajnelson-nist

Copy link
Copy Markdown
Member Author

There's also a line that should go into the PR to have this target develop, not the primary branch.

Oh, nevermind, it's already there. Line 43 of supply-chain.yml.

This PR is ready for review and merge.

@ajnelson-nist
ajnelson-nist marked this pull request as ready for review July 1, 2025 17:43
@ajnelson-nist
ajnelson-nist marked this pull request as draft July 1, 2025 17:45
No effects were observed on Make-managed files.

References:
* #173

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
@ajnelson-nist

Copy link
Copy Markdown
Member Author

This PR is ready for review and merge.

@ajnelson-nist
ajnelson-nist marked this pull request as ready for review July 1, 2025 18:07
This reproduces the effects from PRs 180 and 181.

References:
* #180
* #181

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>

@vulnmaster vulnmaster left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed by Vulnmaster with the assistance of GPT 5.6 High

Thanks for adding an automated dependency-review workflow. I think the general direction is useful, but I found two issues that should be addressed before merge.

  1. The revised Makefile target cannot distinguish formatting changes from the expected .pre-commit-config.yaml change.

After pre-commit autoupdate, .pre-commit-config.yaml is intentionally modified. If pre-commit run returns nonzero, the following unrestricted git diff --stat --exit-code will necessarily fail because it includes that configuration-file change.

As a result, the target reports that formatting changed for any nonzero pre-commit result, including a lint failure, hook installation failure, hook crash, or a source-file modification. Please restructure this logic so that:

  • The exit status from pre-commit run is captured.
  • Source changes are checked separately from .pre-commit-config.yaml.
  • Hook execution failures are not mistaken for formatting changes.
  • The warning says “the update would change file formatting,” rather than “the updated.”

A source-difference check could exclude the expected config change with a pathspec such as:

git diff --stat --exit-code -- . ':(exclude).pre-commit-config.yaml'
  1. The scheduled workflow will not activate while it exists only on develop.

This repository’s default branch is main, and GitHub only triggers scheduled workflows whose workflow files exist on the default branch. Merging this PR into develop alone will therefore not start the nightly check. The explicit ref: develop checkout is appropriate once the workflow reaches main, but please confirm and document how this workflow will be deployed to main.

I also recommend adding workflow_dispatch so the new workflow and Make target can be tested manually. The existing PR CI runs make check, not make check-supply-chain, so the new behavior has not yet been exercised by CI.

Finally, because the workflow automatically downloads and executes updated third-party hooks, please explicitly restrict its token to contents: read; consider setting persist-credentials: false on checkout as well.

With the Makefile logic corrected and the default-branch activation plan confirmed, I would be comfortable approving this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants