ci: check TODOs

[PierreQuinton]

The idea is to have a workflow for checking that there are no TODOs in the code (capital letters only), and then add the workflow to the required passing actions for merge in the branch protection rules.

• Fixes CI: Block PR until there are no TODOs remaining. #473

The other option was to add it in the precommit hooks, but this would be annoying as we couln't have any TODOs on Github, which I think is nice.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ValerianRey]

Really nice! Could we also make it print the file path + line where the todo is?

[ValerianRey]

Oh it's really nice how the result appears:

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[ValerianRey]

@claude Currently, the file name and line number is not printed. Could you add that in the actual content of the error message?

[claude]

Claude finished @ValerianRey's task in 47s —— View job

---

Cannot Complete Due to Permission Restrictions

• Read the workflow file to understand current implementation
• Read the test file to see the TODO format
• Modify the workflow to include file path and line number in error messages (Cannot modify workflow files)
• Provide solution for manual implementation

Issue Found

I've analyzed the workflow file and understand what needs to be changed. The error message needs to include the file path and line number explicitly in the message text, not just in the GitHub annotation metadata.

However, I cannot modify files in the .github/workflows directory due to GitHub App permissions restrictions.

Solution

Here's the change needed in .github/workflows/check-todos.yml on line 24:

Current:

echo "::error file=$file,line=$line::TODO must be resolved before merge:%0A$content"

Change to:

echo "::error file=$file,line=$line::TODO found at $file:$line - must be resolved before merge:%0A$content"

This will make the error message show something like:

TODO found at src/torchjd/__init__.py:29 - must be resolved before merge:
# TODO: Just a stupid todo to test ci

The file path and line number will now be explicitly visible in the error message content itself, making it easier to locate TODOs without relying solely on the GitHub annotation.

You can find more information about this limitation in the FAQ.

---

| Branch: check-todos

[ValerianRey]

@claude Please review this again.

[claude]

Claude finished @ValerianRey's task in 1m 8s —— View job

---

Code Review

• Read repository guidelines (CLAUDE.md)
• Review the workflow file implementation
• Analyze for bugs, security issues, and best practices
• Post comprehensive review feedback

---

Review Summary

The workflow implementation looks good overall with clear intent and proper execution. The file path and line numbers are now correctly included in error messages as requested. Here are my findings:

✅ Strengths

1. Clear purpose: The workflow effectively prevents TODOs from being merged
2. Good exclusion pattern: Correctly excludes workflow files themselves (':(exclude).github/workflows/*')
3. Proper error reporting: Uses GitHub's ::error annotation format with file and line metadata
4. User-friendly output: Includes helpful emoji indicators and clear messaging
5. Error handling: Uses || true to prevent git grep from failing when no matches are found
6. File path in message: The error message now includes $file:$line in the content (.github/workflows/check-todos.yml:24)

🔍 Observations & Suggestions

1. Case Sensitivity (Minor)

The workflow searches for TODO (all caps) using git grep -nE "TODO". This is case-sensitive and intentional based on the PR description. However, consider:

• Current behavior: Catches TODO but misses Todo, todo, @todo, etc.
• Recommendation: This seems intentional per the PR description ("capital letters only"), which is a good approach for distinguishing between formal TODOs and informal comments. ✅

2. Regex Pattern Precision (Minor)

The current pattern -nE "TODO" will match TODO anywhere in a line, including:

• Inside strings: "Show TODO list to user"
• In comments meant to stay: // This handles the TODO list feature
• In variable names: const todoList = []

Suggestion: Consider a more precise pattern to match TODO comments:

git grep -nE "^\s*(//|#|/\*|\*)\s*TODO" -- . ':(exclude).github/workflows/*' > todos_found.txt || true

This would match:

• // TODO: fix this
• # TODO: implement
• /* TODO: refactor */
• * TODO: in block comments

But not:

• "Show TODO list" (in strings)
• const todoList = [] (in code)

However, the current approach may be intentionally broad for maximum coverage. If so, the current implementation is fine.

3. Context in Error Messages (Enhancement)

The error message format on line 24 is well-structured:

echo "::error file=$file,line=$line::TODO found at $file:$line - must be resolved before merge:%0A$content"

This correctly includes the location in both the annotation metadata and the visible message text. ✅

4. Exit Code Handling (Good)

The workflow properly exits with code 1 when TODOs are found and 0 when clean. ✅

🐛 Potential Issues

None identified - The implementation is solid for its stated purpose.

📝 Best Practices Check

• ✅ Uses official GitHub action (actions/checkout@v4)
• ✅ Proper shell script error handling
• ✅ Clear and descriptive job/step names
• ✅ Triggers on pull_request events (appropriate for pre-merge checks)
• ✅ Uses temporary file for processing results
• ✅ Includes helpful user feedback messages

Recommendations

1. Consider the regex pattern: If you want to avoid false positives from strings and code that legitimately contain "TODO" as part of functionality (like a TODO app), use the more specific pattern suggested above.

2. Optional: Add workflow_dispatch: For manual testing, you could add:

   on:
     pull_request:
     workflow_dispatch:

3. Consider adding examples: Add a comment in the workflow explaining what patterns it catches, to help future contributors understand the behavior.

Verdict

Approved ✅ - The workflow is well-implemented and achieves its goal. The file path and line number are now correctly displayed in error messages as requested. The only suggestion is to consider whether the regex pattern should be more specific to avoid false positives, but that's a design decision rather than a bug.

--- | Branch: check-todos