ci: CI check to enforce TODO comment format#1742
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces automated CI enforcement of TODO comment formatting standards for Rust files. It adds a bash script that validates TODO comments follow either // TODO(#NNN): description or // TODO: description format, integrates it into the build system, and updates all existing TODO comments across the codebase to conform to the new standard.
Changes:
- Added new bash script to validate TODO comment format in Rust files
- Created GitHub Actions workflow to run TODO format checks on PRs and pushes
- Updated Makefile to include TODO format check in fast checks
- Standardized 20+ existing TODO comments to follow the required format
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/check-todo-format.sh | New bash script that scans Rust files for TODO comments and validates they follow the required format |
| .github/workflows/todo-format-check.yml | GitHub Actions workflow to run TODO format validation on PR and push events |
| Makefile.toml | Integrated TODO format check into the "fast" task dependencies |
| crates/test-parallel-contract/src/lib.rs | Updated TODO comment to standardized format with issue reference |
| crates/node/src/tee/remote_attestation.rs | Updated TODO comment format with issue number in parentheses |
| crates/node/src/p2p.rs | Updated lowercase todo to uppercase TODO format |
| crates/node/src/keyshare/permanent.rs | Converted non-standard TODO format to required format |
| crates/node/src/indexer/real.rs | Standardized TODO comment formatting |
| crates/node/src/indexer/handler.rs | Updated TODO comment to new format |
| crates/node/src/indexer/fake.rs | Converted lowercase todo to uppercase TODO |
| crates/node/src/config.rs | Updated TODO comments to standardized format with better descriptions |
| crates/node/src/cli.rs | Updated TODO comment with clearer description |
| crates/mpc-attestation/src/attestation.rs | Standardized TODO format with issue reference |
| crates/devnet/src/terraform.rs | Converted multi-line TODO to single-line standardized format |
| crates/contract/tests/sandbox/utils/sign_utils.rs | Updated lowercase todo to uppercase TODO |
| crates/contract/tests/sandbox/utils/consts.rs | Standardized TODO comment format |
| crates/contract/tests/sandbox/upgrade_from_current_contract.rs | Updated TODO format with issue reference |
| crates/contract/tests/sandbox/tee_cleanup_after_resharing.rs | Standardized TODO comment format |
| crates/contract/tests/sandbox/common.rs | Updated TODO format with issue reference |
| crates/contract/src/update.rs | Converted non-standard TODO to required format with better description |
| crates/contract/src/lib.rs | Updated multiple TODO comments to standardized format |
| crates/contract/src/config.rs | Standardized TODO comment format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I’ll need one more review. @barakeinav1 or @DSharifi, whenever you get a moment? |
DSharifi
left a comment
There was a problem hiding this comment.
The script is currently a bit too aggressive. It's flagging whole words if they contain the substring todo. For example "mastodon".
Got this from Gemini to fix it:
#!/usr/bin/env bash
set -euo pipefail
# Define extensions as an array for cleaner handling
CHECKED_EXTENSIONS=(
"*.rs" "*.py" "*.sh" "*.yml" "*.yaml" "*.md"
)
# 1. git ls-files -z uses null-termination for space-safe filenames
# 2. xargs -0 reads those null-terminated strings
# 3. grep \btodo\b looks for the word "todo" specifically
# 4. The final grep -v -E filters out the two valid patterns
INVALID_TODOS=$(git ls-files -z "${CHECKED_EXTENSIONS[@]}" | \
xargs -0 grep -HinE "\btodo\b" | \
grep -vE ":.*(TODO\(#[0-9]+\):|TODO:)" || true)
# Filter out the script itself from the results if it's named similarly
SCRIPT_NAME=$(basename "$0")
INVALID_TODOS=$(echo "$INVALID_TODOS" | grep -v "$SCRIPT_NAME" || true)
if [ -n "$INVALID_TODOS" ]; then
echo "❌ Found TODO comments not matching the required format"
echo ""
echo "Valid formats:"
echo " TODO(#1234): description"
echo " TODO: description"
echo ""
echo "Invalid lines found:"
echo "$INVALID_TODOS"
exit 1
fi
echo "✅ All TODO comments follow the required format"
exit 0
Fixes #1741