Generate and verify cmake target dependency diagram#630
Open
aristarkhovNV wants to merge 5 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces an automated CMake target dependency visualization system. It centralizes CMake build flags into a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
aristarkhovNV
force-pushed
the
aaristarkhov/cmake-layers
branch
from
05d6c35 to
bbb3e38
Compare
aristarkhovNV
force-pushed
the
aaristarkhov/cmake-layers
branch
from
46571d4 to
ffccb62
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
aristarkhovNV
force-pushed
the
aaristarkhov/cmake-layers
branch
from
16b24f6 to
b981a36
Compare
Collaborator
Author
|
/update-cmake-target-layers |
aristarkhovNV
force-pushed
the
aaristarkhov/cmake-layers
branch
from
b981a36 to
4c89631
Compare
Collaborator
Author
|
/update-cmake-target-layers |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/cmake-target-layers.yaml:
- Line 53: Replace mutable GitHub Actions tags with immutable commit SHAs for
supply-chain hardening: locate the three `uses:` entries `actions/checkout@v6`,
`actions/cache@v5`, and `actions/upload-artifact@v6` in the workflow and replace
each tag with the corresponding full commit SHA for the action repo (e.g.,
`actions/checkout@<full-sha>`). Ensure you fetch the correct commit SHA from the
respective official action repository (tags/releases page) and update the YAML
entries so they point to those SHAs instead of `@v*`, then verify the workflow
runs successfully.
In @.github/workflows/update-cmake-target-layers.yaml:
- Around line 68-77: The workflow currently fetches and stores head_sha/head_ref
in the "Get PR info" step (using gh api and writing to GITHUB_OUTPUT) but later
commits to head_ref without revalidating that the PR head still matches
head_sha, allowing a TOCTOU race; fix by re-querying the PR head SHA via gh api
just before making any commit/push (the same logic used in the "Get PR info"
step), compare the fresh SHA to the stored head_sha output, and if they differ
abort (or update the artifact generation to target the new SHA) so you never
write a stale artifact to the latest branch; apply this check in the later
commit steps that perform the push/commit (the steps that consume
head_ref/head_sha).
- Line 117: Replace the mutable tag uses: actions/download-artifact@v7 with a
pinned immutable commit SHA by locating the uses line (uses:
actions/download-artifact@v7) in the workflow and updating it to uses:
actions/download-artifact@<full-commit-sha>; fetch the latest commit SHA from
the actions/download-artifact repository (or the release you intend to track),
verify compatibility, and commit the change so the workflow uses the fixed SHA
instead of the `@v7` tag.
In `@scripts/cmake_target_layers.py`:
- Around line 235-246: _transitive_reach can overflow on cycles because it lacks
an active-path guard; update _transitive_reach to accept and use an "active" set
(or list) tracking the current recursion path, check at the start if node is
already in active and if so raise/return an explicit cycle error including the
path, add node to active before recursing into children and remove it after, and
keep caching behavior intact; apply the same change to the other DFS-like
function referenced (the similar function around lines 249-260) so both detect
and fail fast on cycles with a clear message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a21e53f0-73c9-4873-b892-723ce3fa5a59
📒 Files selected for processing (9)
.github/workflows/build-ubuntu.yml.github/workflows/cmake-target-layers.yaml.github/workflows/update-cmake-target-layers.yamlAGENTS.mdCMakePresets.jsonCMakePresets.json.licensecmake/cmake-structure.mdcmake/cmake-target-layers.mdscripts/cmake_target_layers.py
| runs-on: ubuntu-22.04 | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v6 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the workflow file in question
FILE=".github/workflows/cmake-target-layers.yaml"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "== File: $FILE =="
# Print around the referenced lines (53, 74, 99) with some context
nl -ba "$FILE" | sed -n '35,120p'Repository: NVIDIA/IsaacTeleop
Length of output: 179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/cmake-target-layers.yaml"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "== File: $FILE =="
nl -ba "$FILE" | sed -n '35,120p'Repository: NVIDIA/IsaacTeleop
Length of output: 179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/cmake-target-layers.yaml"
[ -f "$FILE" ] || { echo "Missing file: $FILE"; exit 1; }
echo "== File: $FILE =="
awk -v start=30 -v end=120 '{if(NR>=start && NR<=end) printf "%5d\t%s\n", NR, $0}' "$FILE"Repository: NVIDIA/IsaacTeleop
Length of output: 3951
Pin GitHub Actions uses to immutable commit SHAs (supply-chain hardening).
In .github/workflows/cmake-target-layers.yaml, these uses: use mutable tags—pin each to a full commit SHA instead of @v*:
actions/checkout@v6(line 53)actions/cache@v5(line 74)actions/upload-artifact@v6(line 99)
🧰 Tools
🪛 zizmor (1.25.2)
[error] 53-53: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/cmake-target-layers.yaml at line 53, Replace mutable
GitHub Actions tags with immutable commit SHAs for supply-chain hardening:
locate the three `uses:` entries `actions/checkout@v6`, `actions/cache@v5`, and
`actions/upload-artifact@v6` in the workflow and replace each tag with the
corresponding full commit SHA for the action repo (e.g.,
`actions/checkout@<full-sha>`). Ensure you fetch the correct commit SHA from the
respective official action repository (tags/releases page) and update the YAML
entries so they point to those SHAs instead of `@v*`, then verify the workflow
runs successfully.
Source: Linters/SAST tools
Comment on lines
+68
to
+77
| - name: Get PR info | ||
| id: pr | ||
| run: | | ||
| read -r HEAD_SHA HEAD_REF HEAD_REPO < <( | ||
| gh api "repos/$REPO/pulls/$PR_NUMBER" \ | ||
| --jq '[.head.sha, .head.ref, .head.repo.full_name] | @tsv' | ||
| ) | ||
|
|
||
| echo "head_sha=$HEAD_SHA" >> "$GITHUB_OUTPUT" | ||
| echo "head_ref=$HEAD_REF" >> "$GITHUB_OUTPUT" |
Contributor
There was a problem hiding this comment.
PR head TOCTOU can commit a stale diagram onto a newer branch tip.
The workflow resolves a verify artifact for one HEAD_SHA but later commits to HEAD_REF without revalidating that the PR head is still that SHA. If new commits land mid-run, this can write an outdated artifact onto the latest branch state.
Suggested fix
- name: Commit refreshed diagram via API
id: commit
env:
COMMENTER: ${{ github.event.comment.user.login }}
COMMENTER_ID: ${{ github.event.comment.user.id }}
HEAD_SHA: ${{ steps.pr.outputs.head_sha }}
HEAD_REF: ${{ steps.pr.outputs.head_ref }}
RUN_ID: ${{ steps.find-run.outputs.run_id }}
run: |
+ CURRENT_HEAD_SHA=$(gh api "repos/$REPO/pulls/$PR_NUMBER" --jq '.head.sha')
+ if [ "$CURRENT_HEAD_SHA" != "$HEAD_SHA" ]; then
+ echo "::error::PR head moved from ${HEAD_SHA:0:8} to ${CURRENT_HEAD_SHA:0:8}; rerun /update-cmake-target-layers."
+ exit 1
+ fi
+
# Get blob SHA of the currently-committed file (required to update it via API).
FILE_META=$(gh api "repos/$REPO/contents/cmake/cmake-target-layers.md?ref=$HEAD_REF" \
2>/dev/null || echo '{}')Also applies to: 91-113, 126-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/update-cmake-target-layers.yaml around lines 68 - 77, The
workflow currently fetches and stores head_sha/head_ref in the "Get PR info"
step (using gh api and writing to GITHUB_OUTPUT) but later commits to head_ref
without revalidating that the PR head still matches head_sha, allowing a TOCTOU
race; fix by re-querying the PR head SHA via gh api just before making any
commit/push (the same logic used in the "Get PR info" step), compare the fresh
SHA to the stored head_sha output, and if they differ abort (or update the
artifact generation to target the new SHA) so you never write a stale artifact
to the latest branch; apply this check in the later commit steps that perform
the push/commit (the steps that consume head_ref/head_sha).
| >> "$GITHUB_STEP_SUMMARY" | ||
|
|
||
| - name: Download cmake-target-layers artifact | ||
| uses: actions/download-artifact@v7 |
Contributor
There was a problem hiding this comment.
Pin actions/download-artifact to an immutable commit SHA (Line 117)
This workflow uses a mutable major tag (uses: actions/download-artifact@v7) in a job with write permissions, weakening supply-chain guarantees. Pin the action to a full commit SHA instead of @v7.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 117-117: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/update-cmake-target-layers.yaml at line 117, Replace the
mutable tag uses: actions/download-artifact@v7 with a pinned immutable commit
SHA by locating the uses line (uses: actions/download-artifact@v7) in the
workflow and updating it to uses: actions/download-artifact@<full-commit-sha>;
fetch the latest commit SHA from the actions/download-artifact repository (or
the release you intend to track), verify compatibility, and commit the change so
the workflow uses the fixed SHA instead of the `@v7` tag.
Comment on lines
+235
to
+246
| def _transitive_reach( | ||
| node: str, deps: dict[str, set[str]], cache: dict[str, set[str]] | ||
| ) -> set[str]: | ||
| """All nodes reachable from node at distance >= 1 (node itself excluded).""" | ||
| if node in cache: | ||
| return cache[node] | ||
| result: set[str] = set() | ||
| for child in deps.get(node, set()): | ||
| result.add(child) | ||
| result |= _transitive_reach(child, deps, cache) | ||
| cache[node] = result | ||
| return result |
Contributor
There was a problem hiding this comment.
Transitive-reach DFS can recurse indefinitely on cyclic dependencies.
_transitive_reach has no active-path cycle guard, so a cycle can fail with recursion overflow before the explicit cycle handling path runs. Add an in-progress path set/list in this DFS and fail fast with a cycle message.
Suggested fix
-def _transitive_reach(
- node: str, deps: dict[str, set[str]], cache: dict[str, set[str]]
-) -> set[str]:
+def _transitive_reach(
+ node: str,
+ deps: dict[str, set[str]],
+ cache: dict[str, set[str]],
+ path: list[str] | None = None,
+) -> set[str]:
"""All nodes reachable from node at distance >= 1 (node itself excluded)."""
+ if path is None:
+ path = []
if node in cache:
return cache[node]
+ if node in path:
+ cycle = path[path.index(node):] + [node]
+ raise SystemExit("dependency cycle detected: " + " -> ".join(cycle))
+ path.append(node)
result: set[str] = set()
for child in deps.get(node, set()):
result.add(child)
- result |= _transitive_reach(child, deps, cache)
+ result |= _transitive_reach(child, deps, cache, path)
+ path.pop()
cache[node] = result
return resultAlso applies to: 249-260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/cmake_target_layers.py` around lines 235 - 246, _transitive_reach can
overflow on cycles because it lacks an active-path guard; update
_transitive_reach to accept and use an "active" set (or list) tracking the
current recursion path, check at the start if node is already in active and if
so raise/return an explicit cycle error including the path, add node to active
before recursing into children and remove it after, and keep caching behavior
intact; apply the same change to the other DFS-like function referenced (the
similar function around lines 249-260) so both detect and fail fast on cycles
with a clear message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
Documentation