Add moderation exception to self destruct#7132
Open
Garanas wants to merge 6 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an EmmyLua type for the self-destruct data and replaces the direct callback alias with a validating wrapper that enforces owner authorization, rejects empty selections, and in FullShare mode restricts the action to COMMAND (ACU) units when present before delegating to the core self-destruct function. ChangesSelf-Destruct Callback Validation
Sequence Diagram(s)sequenceDiagram
participant Caller as Callbacks.ToggleSelfDestruct
participant Scenario as ScenarioInfo
participant Filter as CategoryFilter
participant SD as selfdestruct.ToggleSelfDestruct
Caller->>Scenario: read Options.Share
Caller->>Filter: filter units by categories.COMMAND
Note over Filter,Scenario: if FullShare and filtered non-empty -> use filtered\nelse -> use original units
Caller->>SD: ToggleSelfDestruct(data, selectedUnits)
SD-->>Caller: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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.
Actionable comments posted: 1
🤖 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 `@lua/SimCallbacks.lua`:
- Around line 185-207: The callback dereferences units and data.owner before
validation; normalize and validate them up front by calling SecureUnits(units)
into a local variable (e.g., local units = SecureUnits(units)) before any
table.getn or EntityCategoryFilterDown calls, and check that data is non-nil and
data.owner exists (if not, return) rather than accessing data.owner earlier;
then use the normalized units variable in the rest of the function (including
the FullShare branch and the final
import("/lua/selfdestruct.lua").ToggleSelfDestruct calls) to avoid crashes on
malformed payloads.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37d00683-45e8-4e3f-998e-b074075b3cc6
📒 Files selected for processing (2)
lua/SimCallbacks.lualua/selfdestruct.lua
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lua/SimCallbacks.lua (1)
185-207:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
unitsbefore anytable.getn/forwarding.Line 186 still dereferences raw callback input, so a singleton unit/id or any other non-table payload will throw before this wrapper can return safely.
SecureUnitsalready canonicalizes and filters the selection; normalize once up front and reuse that sanitized table in both the FullShare branch and the fallback call.Suggested fix
Callbacks.ToggleSelfDestruct = function(data, units) - -- prevent malformed input - if (not units) or (table.getn(units) == 0) then + -- prevent malformed input + units = SecureUnits(units) + if TableEmpty(units) then return end -- prevent abuse if (not data) or (not data.owner) or (not OkayToMessWithArmy(data.owner)) then @@ -- moderation rule: if you self destruct with one or more ACUs in the selection when playing full -- share, then you only self destruct the ACUs. This does not make it impossible to abuse, but it -- does introduce a simple guardrail. if ScenarioInfo.Options.Share == "FullShare" then - local commandUnits = EntityCategoryFilterDown(categories.COMMAND, SecureUnits(units)) + local commandUnits = EntityCategoryFilterDown(categories.COMMAND, units) if table.getn(commandUnits) > 0 then import("/lua/selfdestruct.lua").ToggleSelfDestruct(data, commandUnits) return end end🤖 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 `@lua/SimCallbacks.lua` around lines 185 - 207, Normalize the incoming selection by calling SecureUnits(units) once at the top (e.g., replace raw uses of units with a local sanitized = SecureUnits(units)), then use that sanitized table for the table.getn check and for all calls to import("/lua/selfdestruct.lua").ToggleSelfDestruct; ensure you perform the early return when the sanitized table is empty and still validate data.owner/OkayToMessWithArmy(data.owner) before invoking ToggleSelfDestruct, and use the same sanitized variable in the ScenarioInfo.Options.Share branch (commandUnits = EntityCategoryFilterDown(categories.COMMAND, sanitized)).
🤖 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.
Duplicate comments:
In `@lua/SimCallbacks.lua`:
- Around line 185-207: Normalize the incoming selection by calling
SecureUnits(units) once at the top (e.g., replace raw uses of units with a local
sanitized = SecureUnits(units)), then use that sanitized table for the
table.getn check and for all calls to
import("/lua/selfdestruct.lua").ToggleSelfDestruct; ensure you perform the early
return when the sanitized table is empty and still validate
data.owner/OkayToMessWithArmy(data.owner) before invoking ToggleSelfDestruct,
and use the same sanitized variable in the ScenarioInfo.Options.Share branch
(commandUnits = EntityCategoryFilterDown(categories.COMMAND, sanitized)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6657cad3-a08f-4d2d-a15e-429c81770c8d
📒 Files selected for processing (3)
changelog/snippets/features.7132.mdchangelog/snippets/graphics.7095.mdlua/SimCallbacks.lua
✅ Files skipped from review due to trivial changes (1)
- changelog/snippets/features.7132.md
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.
Description of the proposed changes
Adds the exception that when you self destruct a selection with one or more ACUs it will only self destruct the ACUs.
Testing done on the proposed changes
Launched the game and verified that the ACU is isolated when I try to self destruct all my units.
Additional context
For this fortunate enough to view it:
Checklist
Summary by CodeRabbit