feat(check_leaks): make memory leak check mandatory to pass CI#578
Merged
Miou-zora merged 2 commits intoApr 9, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe XMake leak-check script now aborts by raising an error when an expected target executable is missing and raises an exception (failing the run) when any memory leaks are detected; behavior when no leaks are found is unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/xmake/check_leaks.lua (1)
11-14:⚠️ Potential issue | 🔴 CriticalMandatory leak gate is still bypassable on prerequisite failures.
If executables are missing or
leaksis unavailable, the task currently prints and continues/returns, which can still end in a green CI result without performing the mandatory check.💡 Proposed fix
@@ - if not os.isfile(bin_path) then - print("Executable not found for target: " .. target_name) - goto continue - end + if not os.isfile(bin_path) then + table.insert(failing_targets, target_name .. " (executable not found)") + goto continue + end @@ local leaks_tool = find_program("leaks", { check = "--help" }) if not leaks_tool then - print("leaks tool not found!") - return + raise("leaks tool not found on macOS runner; cannot perform mandatory memory leak check.") endAlso applies to: 68-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/xmake/check_leaks.lua` around lines 11 - 14, The script currently prints and uses "goto continue" when os.isfile(bin_path) is false (and similarly around the availability of the leaks tool), which lets the mandatory leak gate be bypassed; change those paths in tools/xmake/check_leaks.lua so that when os.isfile(bin_path) or the leaks check fails you abort with a non-zero exit (e.g., call error(...) or os.exit(1)) instead of printing and continuing, and make the same change for the analogous checks around lines 68-71 so missing prerequisites cause the task to fail CI rather than silently pass.
🧹 Nitpick comments (1)
tools/xmake/check_leaks.lua (1)
9-9: Avoid hardcodeddebug; expose taskmodeand resolve binary path from it.Hardcoding
"debug"makes the task fragile outside CI defaults and can silently miss binaries in other modes.♻️ Proposed refactor
@@ -local function check_leaks_macos(targets, leaks_tool, project, os, verbose) +local function check_leaks_macos(targets, leaks_tool, os, verbose, mode) @@ - local bin_path = path.join(os.projectdir(), target:targetdir(), "debug", target:filename()) + local bin_path = path.join(target:targetdir(), mode, target:filename()) @@ import("lib.detect.find_program") + local mode = option.get("mode") or "debug" @@ - failing_targets = check_leaks_macos(targets, leaks_tool, project, os, option.get("verbose")) + failing_targets = check_leaks_macos(targets, leaks_tool, os, option.get("verbose"), mode) @@ options = { + {nil, "mode", "kv", "debug", "Build mode to check (debug/release)"}, {nil, "targets", "vs", nil, "Targets to check for leaks (default: all test targets)"} } }Based on learnings: in
tools/xmake/check_leaks.lua, task context should useoption.get("mode") or "debug"and build the path aspath.join(target:targetdir(), mode, target:filename()).Also applies to: 34-39, 72-72, 90-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/xmake/check_leaks.lua` at line 9, Replace hardcoded "debug" mode when building binary paths by reading the task option mode = option.get("mode") or "debug" and using that variable in path.join; specifically update the bin_path construction (local bin_path = path.join(os.projectdir(), target:targetdir(), "debug", target:filename())) to use path.join(target:targetdir(), mode, target:filename()) (or include os.projectdir() only where appropriate), and apply the same change to the other path.join usages in this file that build binary paths so they all derive the directory from mode instead of the literal "debug"; ensure you declare mode once (e.g., local mode = option.get("mode") or "debug") and reference it in every affected expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tools/xmake/check_leaks.lua`:
- Around line 11-14: The script currently prints and uses "goto continue" when
os.isfile(bin_path) is false (and similarly around the availability of the leaks
tool), which lets the mandatory leak gate be bypassed; change those paths in
tools/xmake/check_leaks.lua so that when os.isfile(bin_path) or the leaks check
fails you abort with a non-zero exit (e.g., call error(...) or os.exit(1))
instead of printing and continuing, and make the same change for the analogous
checks around lines 68-71 so missing prerequisites cause the task to fail CI
rather than silently pass.
---
Nitpick comments:
In `@tools/xmake/check_leaks.lua`:
- Line 9: Replace hardcoded "debug" mode when building binary paths by reading
the task option mode = option.get("mode") or "debug" and using that variable in
path.join; specifically update the bin_path construction (local bin_path =
path.join(os.projectdir(), target:targetdir(), "debug", target:filename())) to
use path.join(target:targetdir(), mode, target:filename()) (or include
os.projectdir() only where appropriate), and apply the same change to the other
path.join usages in this file that build binary paths so they all derive the
directory from mode instead of the literal "debug"; ensure you declare mode once
(e.g., local mode = option.get("mode") or "debug") and reference it in every
affected expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9f85f4a-89cc-469d-b9dd-e0dfac2022e3
📒 Files selected for processing (1)
tools/xmake/check_leaks.lua
|
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
1 participant
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.
Pull Request
Description
Force memory leaks to be resolved in CI
Related Issues (Put "None" if there are no related issues)
close #566
Type of Change
Please delete options that are not relevant.
Changes Made
List the main changes in this PR:
Testing
Describe the tests you ran to verify your changes. Please delete options that are not relevant.
Test Environment
Screenshots/Videos (Put "None" if there are no related issues)
None
Documentation
Please delete options that are not relevant.
Checklist (Don't delete any options)
Breaking Changes (Put "None" if there are no related issues)
None
Additional Notes (Put "None" if there are no related issues)
None
Summary by CodeRabbit