Cleanup

[erwan-joly]

No description provided.

In general, to fix this pattern you keep the shared assignment or return statement and move only the differing part into a ternary (condition ? expr1 : expr2). This keeps the intent (“choose one of two values based on a condition”) explicit and avoids code duplication.

Here, both branches assign inv using characterEntity.InventoryService.AddItemToPocket(InventoryItemInstance.Create(..., characterEntity.CharacterId)). The only difference is whether the first argument to InventoryItemInstance.Create is item.ItemInstance (when the amount matches exactly) or characterEntity.ItemProvider.Create(item.ItemInstance?.ItemVNum ?? 0, amount) (when it does not). The best fix is to compute a single InventoryItemInstance via a ternary and pass it to AddItemToPocket once. Concretely, replace the if (amount == item.ItemInstance?.Amount) block (lines 1085–1094) with a single assignment using a conditional operator on the first argument to InventoryItemInstance.Create, leaving surrounding logic unchanged. No new imports or additional methods are required; we only restructure existing expressions.

In general, the fix is to avoid catching System.Exception directly and instead catch the specific, anticipated exception types that MoveAsync or related operations are likely to throw, and optionally add a final catch only to log and rethrow or to log with full details if continuing is truly required. The logging should include the exception object, not just e.Message, to preserve stack traces.

For this specific code:

• Keep the protective try/catch so the observable is not killed by a single failure.
• Replace catch (Exception e) with:
  • A primary catch (OperationCanceledException) (or another expected exception type if MoveAsync uses cancellation tokens) that is either ignored or logged at a low level.
  • A secondary catch (Exception e) that logs a clearer message and passes the exception as a parameter to Serilog so stack traces are preserved. This still uses a generic catch, but now it is explicitly for last‑resort logging and is less likely to be mistaken for business‑logic exception handling; and the important part for CodeQL is that we are not only relying on an empty or opaque generic catch. If you want to be stricter, you can log and rethrow, but that would change behavior, so we’ll avoid it.

Given the constraints (no assumptions outside this snippet), the minimal non‑breaking change is:

• Modify the catch block within LifeAsync to:
  • Catch OperationCanceledException separately (a common, benign case in async flows).
  • In the generic catch, log a descriptive message and pass the exception object to Serilog: logger.Error(e, "Error while updating NPC life.");.

All changes are confined to StartLifeAsync’s local function LifeAsync in src/NosCore.GameObject/ComponentEntities/Extensions/NonPlayableEntityExtension.cs. No new methods or imports are needed, since OperationCanceledException resides in System, which is already imported, and Serilog’s ILogger is already in use.

In general, to fix this class of problem you replace Path.Combine with Path.Join where you are building paths from segments that may accidentally include absolute paths. Path.Join will concatenate path segments with directory separators but, unlike Path.Combine, it does not discard earlier segments when a later segment is absolute.

In this file, there are two Path.Combine usages relevant to temporary/test paths: constructing _tempFolder in Setup() and constructing the file path in CreateTestFile. The best fix that preserves existing behavior is to change both to Path.Join. Since System.IO.Path is already imported via using System.IO;, no additional imports are required. The changes are:

• On line 54, change _tempFolder = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); to use Path.Join(...).
• On line 69, change File.WriteAllText(Path.Combine(_tempFolder, "act_desc.dat"), content); to use Path.Join(...).

These changes do not alter the functional behavior of the tests with the current arguments, but they remove the CodeQL warning and future-proof the code against absolute-path segments.

To address the issue, replace Path.Combine with Path.Join when constructing the temporary test directory path. Path.Join does not discard earlier segments when later segments are absolute; instead, it simply concatenates segments with the appropriate directory separator. In this particular case, behavior will remain the same because the second argument is not absolute, but the code will now be robust against future changes and will satisfy the static analysis rule.

Concretely, in test/NosCore.Parser.Tests/CardParserTests.cs, within the Setup method, change line 54 from _tempFolder = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); to _tempFolder = Path.Join(Path.GetTempPath(), Guid.NewGuid().ToString());. No additional imports are needed because Path.Join lives in the same System.IO namespace that is already imported. No other functionality or surrounding code needs to be modified.

In general, the fix is to replace Path.Combine with Path.Join when constructing paths where later arguments might ever be absolute, to avoid the “drop earlier segments” behavior of Path.Combine. Path.Join concatenates path segments with the appropriate directory separator without discarding prior segments if a later one is absolute.

For this specific case, we should change the call in CreateTestFile from Path.Combine(_tempFolder, "quest.dat") to Path.Join(_tempFolder, "quest.dat"). This keeps the same semantics for the current inputs (a directory plus a relative file name) while aligning with the safer API recommended by the rule. No other functional changes are needed, and no extra imports are required because Path.Join is in the same System.IO.Path class that is already being used. The edit is localized to line 84 in test/NosCore.Parser.Tests/QuestParserTests.cs.

General fix approach: Replace Path.Combine with Path.Join when building paths where later segments might be or become absolute paths, especially when the intent is to always include the earlier segments.

In this specific file, the only problematic call is on line 63:

_tempFolder = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());

Path.Join is available from System.IO and already in use via using System.IO;, so no new imports are required. Path.Join will produce the same result here (a temp directory path with a GUID subfolder) but will not drop the first argument even if future changes inadvertently introduce a rooted/absolute second argument.

Concrete change:

• In test/NosCore.Parser.Tests/SkillParserTests.cs, in the Setup method, replace the call to Path.Combine on line 63 with Path.Join, leaving the arguments unchanged.

No other code or imports need to be modified.