Skip to content

update: commands#3997

Open
Paco161315 wants to merge 4 commits into
opentibiabr:mainfrom
Paco161315:update-commands
Open

update: commands#3997
Paco161315 wants to merge 4 commits into
opentibiabr:mainfrom
Paco161315:update-commands

Conversation

@Paco161315

@Paco161315 Paco161315 commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

New commands for gods and also some of them recieved update:

1.- Add addon is now up to date.
2.- Add mount is now up to date.
3.- Add skill command has been fixed.
4.- Inbox command has been fixed.
5.- New command to check effects.
6.- New command to cast messages as monster.
7.- New command to cast messages as player.
8.- New command to set outfit looktype.
9.- New command to reset boss storage.
10.- New command to set storages, useful for debug canary.

Everything has been debuged and tested at https://tibiatales.com/

Summary by CodeRabbit

Release Notes

  • New Features

    • Added /z command to trigger magic effects at player location
    • Added /msay command to make players speak as monsters
    • Added /outfit command to change player appearance
    • Added /resetboss command to reset boss cooldowns for players
    • Added /say command for players to speak messages
    • Added /storage command to manage player storage values
  • Improvements

    • Extended /addaddon with additional outfit options
    • Expanded /addmount to support more mount varieties
    • Enhanced /addskill with improved calculation methods
    • Refactored /inbox command with clearer parameter validation

New commands for gods and also some of them recieved update:

1.- Add addon is now up to date.
2.- Add mount is now up to date.
3.- Add skill command has been fixed.
4.- Inbox command has been fixed.
5.- New command to check effects.
6.- New command to cast messages as monster.
7.- New command to cast messages as player.
8.- New command to set outfit look type.
9.- New cocommand to reset boss storage.
10.- New command to set storages, useful for debug canary.

Everything has been debuged and tested at https://tibiatales.com/
@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR extends the god-mode talk action system with six new commands (/z, /msay, /outfit, /say, /storage, /resetboss), refactors existing commands (/addskill, /inbox) with improved validation and logic, and expands outfit and mount data ranges in /addaddon and /addmount.

Changes

God Talk Actions Expansion and Refactor

Layer / File(s) Summary
Data range expansions for outfit and mount commands
data/scripts/talkactions/god/add_addon.lua, data/scripts/talkactions/god/add_mount.lua
Outfit looktype IDs expanded at lines 120–123 and 253–260 in /addaddon, and /addmount mount iteration ceiling increased from 231 to 245.
add_skill command logic refactor
data/scripts/talkactions/god/add_skill.lua
Skill aliases added to skillMap, parameter normalized to lowercase, magic increment rewritten to accumulate required mana per successive level, and generic skill increment changed to iterative per-level delta validation.
inbox_command validation and item management rewrite
data/scripts/talkactions/god/inbox_command.lua
Parameter parsing enhanced with trimming and numeric validation, player and store inbox existence enforced, item removal replaced with reverse-index loop and found-flag tracking, add flow updated with conditional charges attribute, and command metadata updated for optional charges parameter.
Effect visualization command
data/scripts/talkactions/god/effect.lua
New /z talk action parses and validates numeric effect IDs (0–303) and triggers magic effect at player position with cancel messaging on invalid input.
Player message and appearance commands
data/scripts/talkactions/god/monster_say.lua, data/scripts/talkactions/god/say.lua, data/scripts/talkactions/god/outfit.lua
New /msay, /say, and /outfit commands parse comma-separated parameters, resolve target players, enforce ghost-access visibility, and send admin feedback messages. Outfit command supports both numeric looktype and monster name lookup.
Player state modification commands
data/scripts/talkactions/god/storage.lua, data/scripts/talkactions/god/reset_boss.lua
New /storage command reads/writes player storage values with ghost-access check; new /resetboss command resets boss cooldown for target by multi-part name with boss type validation and dual confirmation messages.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • dudantas

Poem

A rabbit hops through scripts so bright, ✨
Six new commands come into sight,
From outfit changes to storage store,
The god-mode tools grow ever more. 🐰
Refactored logic, strong and clean—
Best talk actions ever seen!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'update: commands' is vague and generic, using non-descriptive language that does not convey meaningful information about the specific changes in the changeset. Use a more descriptive title that highlights the primary changes, such as 'Add god commands for effects, outfit, storage, and monster/player messages' or 'Add and update god-level talkaction commands'.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several new god talkactions (/z, /msay, /outfit, /resetboss, /say, /storage) and refactors existing ones (/addaddon, /addmount, /addskill, /inbox). Feedback focuses on resolving critical runtime errors across multiple new scripts where the non-standard string.trimSpace function and undefined globals isPlayerGhost and getPlayerGhostAccess are used; these should be replaced with :trim() and player:canSeeCreature(target) respectively. Additionally, it is recommended to cap the skill increase amount in /addskill to prevent potential server freezes, and to use the global constant CONST_ME_LAST instead of a hardcoded limit in /z.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +17 to +18
local targetName = string.trimSpace(split[1])
local message = string.trimSpace(split[2])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

string.trimSpace is not a standard function in the TFS/Canary API and will cause a runtime error when this command is executed. Use the standard :trim() method on the string instead.

\tlocal targetName = split[1]:trim()\n\tlocal message = split[2]:trim()

return true
end

if isPlayerGhost(target) and getPlayerGhostAccess(target) > getPlayerGhostAccess(player) then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

isPlayerGhost and getPlayerGhostAccess are not defined globally in TFS/Canary and will cause a runtime crash. Use the standard player:canSeeCreature(target) method, which automatically handles ghost mode and access level checks.

\tif not player:canSeeCreature(target) then

Comment on lines +16 to +17
local targetName = string.trimSpace(split[1])
local bossName = string.trimSpace(split[2])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

string.trimSpace is not a standard function in the TFS/Canary API and will cause a runtime error when this command is executed. Use the standard :trim() method on the string instead.

\tlocal targetName = split[1]:trim()\n\tlocal bossName = split[2]:trim()

local bossName = string.trimSpace(split[2])

for i = 3, #split do
bossName = bossName .. ", " .. string.trimSpace(split[i])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Use the standard :trim() method instead of the undefined string.trimSpace function.

\t\tbossName = bossName .. \", \" .. split[i]:trim()

return true
end

if isPlayerGhost(target) and getPlayerGhostAccess(target) > getPlayerGhostAccess(player) then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

isPlayerGhost and getPlayerGhostAccess are not defined globally in TFS/Canary and will cause a runtime crash. Use the standard player:canSeeCreature(target) method, which automatically handles ghost mode and access level checks.

\tif not player:canSeeCreature(target) then

return true
end

if isPlayerGhost(target) and getPlayerGhostAccess(target) > getPlayerGhostAccess(player) then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

isPlayerGhost and getPlayerGhostAccess are not defined globally in TFS/Canary and will cause a runtime crash. Use the standard player:canSeeCreature(target) method, which automatically handles ghost mode and access level checks.

\tif not player:canSeeCreature(target) then

return false
end

local targetName = string.trimSpace(split[1])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

string.trimSpace is not a standard function in the TFS/Canary API and will cause a runtime error when this command is executed. Use the standard :trim() method on the string instead.

\tlocal targetName = split[1]:trim()

player:sendCancelMessage("Player " .. targetName .. " not found.")
return true
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

isPlayerGhost and getPlayerGhostAccess are not defined globally in TFS/Canary and will cause a runtime crash. Use the standard player:canSeeCreature(target) method, which automatically handles ghost mode and access level checks.

\tif not player:canSeeCreature(target) then


local skillParam = split[2]:trim()
local skillParam = split[2]:trim():lower()
local skillIncreaseAmount = tonumber(split[3]) or 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

To prevent potential server freezes or hangs caused by extremely large inputs (e.g., a typo adding 100,000+ levels), it is highly recommended to enforce a reasonable upper limit on the skillIncreaseAmount.

\tlocal skillIncreaseAmount = math.min(tonumber(split[3]) or 1, 1000)

Comment on lines +11 to +14
if effectId < 0 or effectId > 303 then
player:sendCancelMessage("Effect ID must be between 0 and 303.")
return true
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Instead of hardcoding 303 as the maximum effect ID, use the global constant CONST_ME_LAST. This ensures the command automatically supports any new magic effects added to the server or client without requiring manual updates to this script.

    if effectId < 0 or effectId > CONST_ME_LAST then\n        player:sendCancelMessage(\"Effect ID must be between 0 and \" .. CONST_ME_LAST .. \".\")\n        return true\n    end

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (6)
data/scripts/talkactions/god/inbox_command.lua (1)

64-64: 💤 Low value

Message mentions charges even when not set.

The success message always includes "with %d charges", even when charges is 0 (not specified or explicitly 0). Since the charges attribute is only set when charges > 0 (lines 61-63), the message should reflect this.

♻️ Proposed fix for clearer messaging
-			player:sendTextMessage(MESSAGE_LOOK, string.format("Added item %d to %s's inbox with %d charges.", itemId, targetName, charges))
+			local msg = charges > 0 and string.format("Added item %d to %s's inbox with %d charges.", itemId, targetName, charges) or string.format("Added item %d to %s's inbox.", itemId, targetName)
+			player:sendTextMessage(MESSAGE_LOOK, msg)
🤖 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 `@data/scripts/talkactions/god/inbox_command.lua` at line 64, The success
message always appends "with %d charges" even when charges is 0; update the
post-add message (the player:sendTextMessage call that uses itemId, targetName,
charges) to only include the "with X charges" clause when charges > 0 (the same
condition used earlier when setting the item's charges). Implement this by
composing two message variants (one including "with %d charges" and one without)
and invoking player:sendTextMessage with the appropriate formatted string based
on the charges value.
data/scripts/talkactions/god/outfit.lua (2)

26-29: 💤 Low value

Cache trimmed target name to avoid redundant calls.

The expression t[2]:trim() is evaluated three times (lines 26, 27, 29). Cache the trimmed value once for clarity and minor efficiency gain.

♻️ Proposed refactor
 	local target = player
 	if t[2] and t[2]:trim() ~= "" then
-		target = Player(t[2]:trim())
+		local targetName = t[2]:trim()
+		target = Player(targetName)
 		if not target then
-			player:sendTextMessage(MESSAGE_ADMINISTRATOR, "Player '" .. t[2]:trim() .. "' not found.")
+			player:sendTextMessage(MESSAGE_ADMINISTRATOR, "Player '" .. targetName .. "' not found.")
 			return true
 		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 `@data/scripts/talkactions/god/outfit.lua` around lines 26 - 29, Cache the
trimmed target name once instead of calling t[2]:trim() repeatedly: create a
local variable (e.g., local name = t[2]:trim()) after checking t[2], then use
name when constructing Player(name), when testing for empty string, and in the
player:sendTextMessage(MESSAGE_ADMINISTRATOR, ...) message and any other places
referencing t[2]:trim(); update uses of target = Player(...) accordingly.

10-10: Standardize whitespace trimming helper usage in outfit.lua
:trim() is valid in this codebase (it’s used widely, including in data/libs/functions/string.lua via v:trim() / firstPart:trim()), so there’s no compatibility issue; the main inconsistency is style since several other god talkactions use string.trimSpace() instead. Consider standardizing on one helper across these scripts for readability/maintainability.

🤖 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 `@data/scripts/talkactions/god/outfit.lua` at line 10, The line setting
lookParam uses the :trim() helper (local lookParam = t[1] and t[1]:trim() or "")
while other god talkactions use string.trimSpace(); update this to the
project-standard helper by replacing the :trim() call with string.trimSpace(...)
so it reads consistent with other scripts—edit the assignment for lookParam in
outfit.lua (the variable lookParam and input t[1]) to use string.trimSpace to
normalize whitespace handling across god talkactions.
data/scripts/talkactions/god/storage.lua (1)

16-18: 💤 Low value

Trim whitespace before converting storage value.

Line 18 converts split[3] to a number without trimming whitespace, unlike targetName on line 16. While tonumber() may handle whitespace, explicit trimming ensures consistency with the other parameters.

♻️ Proposed refactor for consistency
 	local targetName = string.trimSpace(split[1])
-	local storageKey = tonumber(split[2])
-	local storageValue = split[3] and tonumber(split[3])
+	local storageKey = tonumber(string.trimSpace(split[2]))
+	local storageValue = split[3] and tonumber(string.trimSpace(split[3]))
🤖 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 `@data/scripts/talkactions/god/storage.lua` around lines 16 - 18, The
storageValue conversion is missing a whitespace trim and should mirror
targetName's handling; update the assignment for storageValue in storage.lua to
trim split[3] before calling tonumber (e.g., use string.trimSpace on split[3])
so storageValue is assigned from tonumber(trimmedValue) and remains nil-safe
like storageKey and targetName.
data/scripts/talkactions/god/effect.lua (1)

11-14: Replace the hardcoded 303 with the engine’s effect enum bound
MagicEffectClasses’ last defined magic effect is CONST_ME_PURPLE_ELECTRIC_SPARK = 303, and the engine treats valid IDs as < CONST_ME_LAST, so the current range check is consistent. Still, avoid the magic number by using CONST_ME_LAST (e.g., effectId >= CONST_ME_LAST) in data/scripts/talkactions/god/effect.lua (lines 11-14).

🤖 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 `@data/scripts/talkactions/god/effect.lua` around lines 11 - 14, The range
check for effectId uses the hardcoded 303; change it to use the engine constant
by replacing the comparison with CONST_ME_LAST (e.g., validate with "effectId <
0 or effectId >= CONST_ME_LAST") in the god talk action where effectId is
checked so it uses the engine's MagicEffectClasses bound (references: effectId,
CONST_ME_LAST).
data/scripts/talkactions/god/add_skill.lua (1)

42-42: ⚡ Quick win

Validate that skillIncreaseAmount is positive.

The command does not validate that the amount is a positive integer. A value of 0 or negative numbers could produce unexpected results:

  • For "level", negative amounts would reduce the player's level (if intended, this is fine, but should be documented).
  • For "magic" and skill types, loops would not execute but success messages would still be sent, misleading the admin.

Consider adding validation:

local skillIncreaseAmount = tonumber(split[3]) or 1
if skillIncreaseAmount <= 0 then
	player:sendCancelMessage("Amount must be a positive number.")
	return true
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 `@data/scripts/talkactions/god/add_skill.lua` at line 42, The
skillIncreaseAmount parsed from split[3] needs validation to ensure it's a
positive integer; in add_skill.lua after computing skillIncreaseAmount (the
local variable skillIncreaseAmount) check that tonumber returned a number and
that skillIncreaseAmount > 0 (optionally floor it to an integer) and if not call
player:sendCancelMessage("Amount must be a positive number.") and return true to
abort the command so no misleading success messages are sent.
🤖 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 `@data/scripts/talkactions/god/inbox_command.lua`:
- Around line 55-67: The removal/add branches inconsistently continue to return
true on operation failure; update the handler so failure branches explicitly
return false: after the "if not found then player:sendTextMessage(...)" branch
(the remove action that checks the local found variable) add a return false, and
after the "else" branch where target:addItemStoreInbox(...) returns nil and the
code calls player:sendTextMessage(...) for the add action add a return false;
this makes failure responses for found/item consistent with other validation
checks like the earlier returns false.
- Line 18: Trim params[4] before converting to number to match the behavior used
for params[3]; specifically, update the charges assignment (the local variable
charges) to call :trim() on params[4] with a nil-check (avoid calling :trim() on
nil) and then pass the trimmed string into tonumber(... ) or default to 0 if
tonumber returns nil.

In `@data/scripts/talkactions/god/monster_say.lua`:
- Around line 11-18: The message currently takes only split[2], truncating
anything after the first comma; change the assignment so after extracting
targetName from split[1] you rebuild the message from the remaining parts (e.g.,
join split[2]..split[`#split`] with commas) and then trim it; locate the
param:split usage and the lines setting targetName and message and replace the
single-index read of split[2] with a reconstruction using the rest of the split
array (similar to the pattern used in reset_boss.lua), ensuring you still call
string.trimSpace on the final message.

In `@data/scripts/talkactions/god/outfit.lua`:
- Around line 25-32: Add a ghost-access comparison after resolving target (the
target variable created with Player(t[2]:trim())): call the same ghost-access
check used in other commands (compare player:getGhostAccess() and
target:getGhostAccess(), or the project's equivalent accessor) and if the
invoking player's ghost access is lower than the target's, send a warning via
player:sendTextMessage and return true; insert this check immediately after the
target resolution block so /outfit enforces the same ghost-access restriction as
/msay, /say, /storage, and /resetboss.

In `@data/scripts/talkactions/god/say.lua`:
- Around line 11-18: The current parsing uses param:split(",") then assigns
message = string.trimSpace(split[2]), which drops any commas inside the message;
change the logic after splitting so targetName = string.trimSpace(split[1])
stays the same but rebuild message by concatenating all remaining parts
(split[2]..split[`#split`]) with commas and then applying string.trimSpace —
follow the same multi-part reconstruction pattern used in reset_boss.lua (use
table.concat on the tail of split, then trim) to preserve commas in the message.

---

Nitpick comments:
In `@data/scripts/talkactions/god/add_skill.lua`:
- Line 42: The skillIncreaseAmount parsed from split[3] needs validation to
ensure it's a positive integer; in add_skill.lua after computing
skillIncreaseAmount (the local variable skillIncreaseAmount) check that tonumber
returned a number and that skillIncreaseAmount > 0 (optionally floor it to an
integer) and if not call player:sendCancelMessage("Amount must be a positive
number.") and return true to abort the command so no misleading success messages
are sent.

In `@data/scripts/talkactions/god/effect.lua`:
- Around line 11-14: The range check for effectId uses the hardcoded 303; change
it to use the engine constant by replacing the comparison with CONST_ME_LAST
(e.g., validate with "effectId < 0 or effectId >= CONST_ME_LAST") in the god
talk action where effectId is checked so it uses the engine's MagicEffectClasses
bound (references: effectId, CONST_ME_LAST).

In `@data/scripts/talkactions/god/inbox_command.lua`:
- Line 64: The success message always appends "with %d charges" even when
charges is 0; update the post-add message (the player:sendTextMessage call that
uses itemId, targetName, charges) to only include the "with X charges" clause
when charges > 0 (the same condition used earlier when setting the item's
charges). Implement this by composing two message variants (one including "with
%d charges" and one without) and invoking player:sendTextMessage with the
appropriate formatted string based on the charges value.

In `@data/scripts/talkactions/god/outfit.lua`:
- Around line 26-29: Cache the trimmed target name once instead of calling
t[2]:trim() repeatedly: create a local variable (e.g., local name = t[2]:trim())
after checking t[2], then use name when constructing Player(name), when testing
for empty string, and in the player:sendTextMessage(MESSAGE_ADMINISTRATOR, ...)
message and any other places referencing t[2]:trim(); update uses of target =
Player(...) accordingly.
- Line 10: The line setting lookParam uses the :trim() helper (local lookParam =
t[1] and t[1]:trim() or "") while other god talkactions use string.trimSpace();
update this to the project-standard helper by replacing the :trim() call with
string.trimSpace(...) so it reads consistent with other scripts—edit the
assignment for lookParam in outfit.lua (the variable lookParam and input t[1])
to use string.trimSpace to normalize whitespace handling across god talkactions.

In `@data/scripts/talkactions/god/storage.lua`:
- Around line 16-18: The storageValue conversion is missing a whitespace trim
and should mirror targetName's handling; update the assignment for storageValue
in storage.lua to trim split[3] before calling tonumber (e.g., use
string.trimSpace on split[3]) so storageValue is assigned from
tonumber(trimmedValue) and remains nil-safe like storageKey and targetName.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ef340cbb-224d-4d99-9a16-2cc4c889acea

📥 Commits

Reviewing files that changed from the base of the PR and between 44f1c2f and ec53af3.

📒 Files selected for processing (10)
  • data/scripts/talkactions/god/add_addon.lua
  • data/scripts/talkactions/god/add_mount.lua
  • data/scripts/talkactions/god/add_skill.lua
  • data/scripts/talkactions/god/effect.lua
  • data/scripts/talkactions/god/inbox_command.lua
  • data/scripts/talkactions/god/monster_say.lua
  • data/scripts/talkactions/god/outfit.lua
  • data/scripts/talkactions/god/reset_boss.lua
  • data/scripts/talkactions/god/say.lua
  • data/scripts/talkactions/god/storage.lua

local targetName = params[1]:trim()
local action = params[2]:lower():trim()
local itemId = tonumber(params[3]:trim())
local charges = tonumber(params[4]) or 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Trim whitespace before converting charges to number.

Line 17 trims params[3] before calling tonumber, but line 18 does not trim params[4]. If the charges parameter contains leading or trailing whitespace (e.g., " 10 "), tonumber may fail and default to 0 unexpectedly.

🔧 Proposed fix
-	local charges = tonumber(params[4]) or 0
+	local charges = tonumber(params[4] and params[4]:trim()) or 0

Note: Added nil-check for params[4] to prevent calling :trim() on nil.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
local charges = tonumber(params[4]) or 0
local charges = tonumber(params[4] and params[4]:trim()) or 0
🤖 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 `@data/scripts/talkactions/god/inbox_command.lua` at line 18, Trim params[4]
before converting to number to match the behavior used for params[3];
specifically, update the charges assignment (the local variable charges) to call
:trim() on params[4] with a nil-check (avoid calling :trim() on nil) and then
pass the trimmed string into tonumber(... ) or default to 0 if tonumber returns
nil.

Comment on lines +55 to 67
if not found then
player:sendTextMessage(MESSAGE_LOOK, string.format("Item %d not found in %s's inbox.", itemId, targetName))
end
elseif action == "add" then
local item = target:addItemStoreInbox(itemId, 1, true, false)
if item then
if charges > 0 then
item:setAttribute(ITEM_ATTRIBUTE_CHARGES, charges)
end
player:sendTextMessage(MESSAGE_LOOK, string.format("Added item %d to %s's inbox with %d charges.", itemId, targetName, charges))
else
player:sendTextMessage(MESSAGE_LOOK, string.format("Failed to add item %d to %s's inbox.", itemId, targetName))
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Inconsistent return value for operation failures.

When an item is not found during removal (lines 55-57) or when adding an item fails (line 66), the command sends an error message but continues to return true (line 73). This is inconsistent with validation errors (lines 6, 12, 22, 28, 34, 41, 70) which all return false.

This creates ambiguity: should operation failures return false like validation errors, or return true to indicate the command executed without error (even though the operation had no effect)?

Consider standardizing the return value behavior. If operation failures should return false, add explicit returns after lines 57 and 66.

🔧 Proposed fix to return false on operation failures
 		if not found then
 			player:sendTextMessage(MESSAGE_LOOK, string.format("Item %d not found in %s's inbox.", itemId, targetName))
+			return false
 		end
 	elseif action == "add" then
 		local item = target:addItemStoreInbox(itemId, 1, true, false)
 		if item then
 			if charges > 0 then
 				item:setAttribute(ITEM_ATTRIBUTE_CHARGES, charges)
 			end
 			player:sendTextMessage(MESSAGE_LOOK, string.format("Added item %d to %s's inbox with %d charges.", itemId, targetName, charges))
 		else
 			player:sendTextMessage(MESSAGE_LOOK, string.format("Failed to add item %d to %s's inbox.", itemId, targetName))
+			return false
 		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 `@data/scripts/talkactions/god/inbox_command.lua` around lines 55 - 67, The
removal/add branches inconsistently continue to return true on operation
failure; update the handler so failure branches explicitly return false: after
the "if not found then player:sendTextMessage(...)" branch (the remove action
that checks the local found variable) add a return false, and after the "else"
branch where target:addItemStoreInbox(...) returns nil and the code calls
player:sendTextMessage(...) for the add action add a return false; this makes
failure responses for found/item consistent with other validation checks like
the earlier returns false.

Comment on lines +11 to +18
local split = param:split(",")
if #split < 2 then
player:sendCancelMessage(usage)
return false
end

local targetName = string.trimSpace(split[1])
local message = string.trimSpace(split[2])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Message content is truncated after the first comma.

Line 18 assigns only split[2] to the message. If the message contains commas, all content after the first comma is silently discarded.

Example:

  • Command: /msay Player,Hello, world!
  • Expected message: "Hello, world!"
  • Actual message: "Hello"
🐛 Proposed fix to concatenate remaining message parts
 	local targetName = string.trimSpace(split[1])
-	local message = string.trimSpace(split[2])
+	local message = string.trimSpace(split[2] or "")
+	for i = 3, `#split` do
+		message = message .. "," .. string.trimSpace(split[i])
+	end

This pattern matches the multi-part name reconstruction in reset_boss.lua lines 19-21.

🤖 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 `@data/scripts/talkactions/god/monster_say.lua` around lines 11 - 18, The
message currently takes only split[2], truncating anything after the first
comma; change the assignment so after extracting targetName from split[1] you
rebuild the message from the remaining parts (e.g., join split[2]..split[`#split`]
with commas) and then trim it; locate the param:split usage and the lines
setting targetName and message and replace the single-index read of split[2]
with a reconstruction using the rest of the split array (similar to the pattern
used in reset_boss.lua), ensuring you still call string.trimSpace on the final
message.

Comment on lines +25 to +32
local target = player
if t[2] and t[2]:trim() ~= "" then
target = Player(t[2]:trim())
if not target then
player:sendTextMessage(MESSAGE_ADMINISTRATOR, "Player '" .. t[2]:trim() .. "' not found.")
return true
end
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Missing ghost access check for target player.

All other player-targeting commands in this PR (/msay, /say, /storage, /resetboss) enforce ghost access restrictions, but /outfit does not. This allows admins with lower ghost access to modify the appearance of higher-privileged ghost players.

🔒 Proposed fix to add ghost access check
 	local target = player
 	if t[2] and t[2]:trim() ~= "" then
 		target = Player(t[2]:trim())
 		if not target then
 			player:sendTextMessage(MESSAGE_ADMINISTRATOR, "Player '" .. t[2]:trim() .. "' not found.")
 			return true
 		end
+		if isPlayerGhost(target) and getPlayerGhostAccess(target) > getPlayerGhostAccess(player) then
+			player:sendTextMessage(MESSAGE_ADMINISTRATOR, "Player '" .. t[2]:trim() .. "' not found.")
+			return true
+		end
 	end

This pattern matches the ghost access enforcement in the other commands (e.g., monster_say.lua lines 26-29).

🤖 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 `@data/scripts/talkactions/god/outfit.lua` around lines 25 - 32, Add a
ghost-access comparison after resolving target (the target variable created with
Player(t[2]:trim())): call the same ghost-access check used in other commands
(compare player:getGhostAccess() and target:getGhostAccess(), or the project's
equivalent accessor) and if the invoking player's ghost access is lower than the
target's, send a warning via player:sendTextMessage and return true; insert this
check immediately after the target resolution block so /outfit enforces the same
ghost-access restriction as /msay, /say, /storage, and /resetboss.

Comment on lines +11 to +18
local split = param:split(",")
if #split < 2 then
player:sendCancelMessage(usage)
return false
end

local targetName = string.trimSpace(split[1])
local message = string.trimSpace(split[2])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Message content is truncated after the first comma.

Line 18 assigns only split[2] to the message. If the message contains commas, all content after the first comma is silently discarded.

Example:

  • Command: /say Player,Hello, world!
  • Expected message: "Hello, world!"
  • Actual message: "Hello"
🐛 Proposed fix to concatenate remaining message parts
 	local targetName = string.trimSpace(split[1])
-	local message = string.trimSpace(split[2])
+	local message = string.trimSpace(split[2] or "")
+	for i = 3, `#split` do
+		message = message .. "," .. string.trimSpace(split[i])
+	end

This pattern matches the multi-part name reconstruction in reset_boss.lua lines 19-21.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
local split = param:split(",")
if #split < 2 then
player:sendCancelMessage(usage)
return false
end
local targetName = string.trimSpace(split[1])
local message = string.trimSpace(split[2])
local split = param:split(",")
if `#split` < 2 then
player:sendCancelMessage(usage)
return false
end
local targetName = string.trimSpace(split[1])
local message = string.trimSpace(split[2] or "")
for i = 3, `#split` do
message = message .. "," .. string.trimSpace(split[i])
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 `@data/scripts/talkactions/god/say.lua` around lines 11 - 18, The current
parsing uses param:split(",") then assigns message = string.trimSpace(split[2]),
which drops any commas inside the message; change the logic after splitting so
targetName = string.trimSpace(split[1]) stays the same but rebuild message by
concatenating all remaining parts (split[2]..split[`#split`]) with commas and then
applying string.trimSpace — follow the same multi-part reconstruction pattern
used in reset_boss.lua (use table.concat on the tail of split, then trim) to
preserve commas in the message.

@sonarqubecloud

sonarqubecloud Bot commented Jun 7, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant