Open
Conversation
📝 WalkthroughWalkthroughReplaces CLI multi-arg invocation with a config-file-driven workflow: adds a Config dataclass to load/normalize JSON settings, modifies patcher entrypoint to accept a single config path, reworks hook/section generation to include address annotations, and adds example config and linker script files. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as main.py
participant Config as Config.load_from_json
participant Patcher as Patcher.patch
participant Hook as Hook.load_hook
participant Linker as Compiler/Linker
CLI->>Config: load_from_json(config_path)
Config-->>CLI: Config instance (paths, flags)
CLI->>Patcher: patch(config_path)
Patcher->>Config: read input/output/toolchain
Patcher->>Hook: load_hook(hook_file, addresses)
Hook-->>Patcher: Hook with sections (headers + address comments)
Patcher->>Patcher: create_sections_file(input_path, output_path, addresses)
Patcher->>Linker: run_process(compile/link commands)
Linker-->>Patcher: compile/link results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patcher/PEData.py (1)
75-79:⚠️ Potential issue | 🟡 MinorReturn type annotation is inconsistent with actual behavior.
The function signature declares
Optional[PESect]but now raises an exception instead of returningNone. This type hint mismatch will mislead callers and static analysis tools.Proposed fix
- def find_sect(self, name: str) -> Optional[PESect]: + def find_sect(self, name: str) -> PESect: for sect in self.sects: if sect.name == name: return sect raise Exception(f"Couldn't find section {name}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patcher/PEData.py` around lines 75 - 79, The signature for find_sect declares Optional[PESect] but the implementation raises on miss; make behavior and annotation consistent by changing find_sect to return None when no section is found instead of raising—update the method body in the find_sect function to return None (preserving the loop and comparison against sect.name) and leave the return type as Optional[PESect]; ensure callers that relied on exceptions handle a None result where find_sect is used.
🧹 Nitpick comments (8)
patcher/Hook.py (3)
21-28: Nested function is redefined on each loop iteration.
replace_addressis defined inside the loop but only depends onself._addresseswhich doesn't change. Moving it outside the loop would be slightly more efficient.Proposed refactor
def lines_to_cpp(self): s = [] + + def replace_address(match: re.Match[str]) -> str: + func_name = match.group(1) + if func_name in self._addresses: + return f'{match.group(1)} /* {self._addresses[func_name]} */' + return match.group(0) + for line in self._lines: - # line = line.translate(ESCAPE_TRANSLATION) - - def replace_address(match: re.Match[str]) -> str: - func_name = match.group(1) - if func_name in self._addresses: - return f'{match.group(1)} /* {self._addresses[func_name]} */' - return match.group(0) - if FUNCTION_NAME_RE.findall(line): line = FUNCTION_NAME_RE.subn(replace_address, line)[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patcher/Hook.py` around lines 21 - 28, The nested function replace_address is recreated on every loop iteration but only reads self._addresses, so move its definition out of the loop (e.g., make it a private method on the class or define it once above the loop) and keep the same signature (accepting a re.Match and returning str); update the code that calls FUNCTION_NAME_RE.subn(replace_address, line) to use the new standalone method/function so the behavior is unchanged while avoiding repeated redefinition.
6-6: Unused constantESCAPE_TRANSLATION.This constant is defined but never used since line 19 is commented out. Consider removing both if the escape logic is no longer needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patcher/Hook.py` at line 6, The constant ESCAPE_TRANSLATION is defined but never used (the escape logic in Hook, referenced by ESCAPE_TRANSLATION, is commented out); remove the unused constant ESCAPE_TRANSLATION and also remove the now-dead commented escape logic (or alternatively fully restore and use ESCAPE_TRANSLATION where the escape was intended) so the file no longer contains unused definitions—look for ESCAPE_TRANSLATION and the commented escape block in Hook.py and either delete both or re-enable the escape code to use the constant.
19-19: Remove commented-out code.This commented-out line appears to be dead code. If the escape translation is intentionally disabled, remove it along with the unused
ESCAPE_TRANSLATIONconstant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patcher/Hook.py` at line 19, Remove the dead commented-out escape translation by deleting the commented line containing "line = line.translate(ESCAPE_TRANSLATION)" and then remove the now-unused ESCAPE_TRANSLATION constant (and any related import or references) from Hook.py; ensure no other code references ESCAPE_TRANSLATION before deleting and run tests/linters to confirm there are no remaining unused symbol errors.patcher/Patcher.py (4)
309-310: Deprecation warning lacks guidance on migration.The warning about deprecated
.cpphook files doesn't provide actionable guidance. Consider including a reference to documentation or instructions for converting to.hookformat.Proposed enhancement
for hook_path in list_files_at(hooks_folder_path, "**/*.cpp"): - print(f"Hook file {hook_path} is deprecated, use .hook file instead") + print(f"WARNING: Hook file {hook_path} is deprecated. Convert to .hook format. See README.md for details.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patcher/Patcher.py` around lines 309 - 310, The deprecation message printed in the loop that iterates over list_files_at(hooks_folder_path, "**/*.cpp") should be expanded to include actionable migration guidance; update the print in that loop (inside Patcher.py where list_files_at and hooks_folder_path are used) to tell users to rename/convert to .hook files and point them to the relevant docs or conversion instructions (e.g., add a brief conversion note and a URL or path to documentation). Ensure the message remains concise and includes the .cpp → .hook migration step and where to find more details.
220-221: UseTypeErrorfor type validation.When checking argument types,
TypeErroris more appropriate than a genericException.Proposed fix
if not isinstance(template, Template): - raise Exception("Not a template") + raise TypeError("Expected a Template instance")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patcher/Patcher.py` around lines 220 - 221, Replace the generic Exception raised when the parameter "template" is not an instance of Template with a TypeError to correctly signal a type validation failure: in the check that currently reads "if not isinstance(template, Template): raise Exception(...)" update it to raise TypeError with a clear message that includes the expected type and the actual type of template (e.g., "template must be a Template, got <type>"); locate this check around the Template usage in Patcher.py (the Template class and the template variable) and make the change there.
312-314: Same formatting issue - multiple statements on one line.Proposed fix
if run_system( t"""cd {build_hooks_folder} & - {config.gcc_path} -c {" ".join(config.asm_flags)} {hooks_folder_path / "*.cpp"}"""): raise Exception("Errors occurred during building of hooks files") + {config.gcc_path} -c {" ".join(config.asm_flags)} {hooks_folder_path / "*.cpp"}"""): + raise Exception("Errors occurred during building of hooks files")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patcher/Patcher.py` around lines 312 - 314, The if-statement combines the conditional and the raise on one line causing formatting/PEP8 issues; modify the block around run_system(...) so the raise Exception is on its own indented line under the if, e.g. keep the run_system(...) call as the if condition (referencing run_system, build_hooks_folder, config.gcc_path, config.asm_flags, hooks_folder_path) and place a properly indented raise Exception("Errors occurred during building of hooks files") on the next line.
306-307: Multiple statements on one line reduce readability.The
ifstatement andraiseare on the same line, making the code harder to read.Proposed fix
if run_system( t"""cd {build_hooks_folder} & - {config.gcc_path} -c {" ".join(config.asm_flags)} {build_hooks_folder / "*.cpp"}"""): raise Exception( - "Errors occurred during building of generated hooks files") + {config.gcc_path} -c {" ".join(config.asm_flags)} {build_hooks_folder / "*.cpp"}"""): + raise Exception("Errors occurred during building of generated hooks files")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patcher/Patcher.py` around lines 306 - 307, The if-statement and the raise are jammed on one line; in the function containing the call that builds generated hooks (the expression referencing config.gcc_path, config.asm_flags and build_hooks_folder), split the conditional and the exception into a normal multi-line if block: place the raise Exception(...) on the next indented line beneath the if so the condition and the error handling are separated and readable.patcher/Config.py (1)
52-62: Validation order may reject relative build paths unexpectedly.If a user provides a relative
build_folder_pathin the config, the validation at line 60 will reject it even though relative paths work for other settings. However, the default (when omitted) correctly resolves to an absolute path.Consider documenting this requirement or resolving relative build paths against target_folder_path for consistency.
Optional: resolve relative build paths
self.build_folder_path = Path(self.build_folder_path)\ if self.build_folder_path \ else self.target_folder_path / "build" + if not self.build_folder_path.is_absolute(): + self.build_folder_path = self.target_folder_path / self.build_folder_path + - if not self.build_folder_path.is_absolute(): - raise ValueError( - "build_folder_path must be an absolute path to folder")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patcher/Config.py` around lines 52 - 62, The validation currently rejects any non-absolute self.build_folder_path in Config.py (the check after setting self.build_folder_path), which prevents users from supplying relative build_folder_path even though defaults are resolved; change the logic in the Config initializer so that if build_folder_path is provided as a relative path you resolve it against self.target_folder_path (e.g., Path(self.target_folder_path) / self.build_folder_path) before running the is_absolute() check, or alternatively remove the strict absolute check and document that relative build_folder_path is resolved against target_folder_path; update places referencing build_folder_path, and keep the symbols self.build_folder_path and self.target_folder_path consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Line 7: The current invocation uses argument unpacking
patcher.patch(*sys.argv[1:]) which will pass multiple CLI args to patcher.patch
(now expecting a single config_path) and causes a TypeError; change it to pass
only the first CLI argument (e.g., use sys.argv[1] with a sensible default or
validate arg count) so that patcher.patch receives a single config_path string;
update any related error handling to surface a clear message when no argument is
provided.
In `@patcher/Patcher.py`:
- Around line 219-235: The run_system function builds a shell command string and
calls os.system which is vulnerable to shell injection; change run_system to
build a list of arguments (iterate template parts in run_system, convert
Path/part.value to str without manual quoting) and call subprocess.run(args,
check=False, shell=False) (or subprocess.run(args, cwd=..., capture_output=...
if you need directory changes handled via a "cd" token) and return
CompletedProcess.returncode; ensure you stop using os.system and string
concatenation, keep the Template/Path handling but produce a list of argv
elements and pass shell=False to subprocess.run for safe execution.
In `@README.md`:
- Around line 17-72: The README's JSON example contains inline // comments and a
trailing comma after the "asm_flags" block which makes it invalid JSON; update
the code fence from ```json to ```jsonc to indicate JSON with comments and
remove the trailing comma after the final "asm_flags" array (or alternatively
move all explanatory comments outside the code block) so consumers can
copy/paste without parse errors—look for the example object containing
"target_folder_path", "clang", "clang_flags", "gcc_flags", and "asm_flags".
---
Outside diff comments:
In `@patcher/PEData.py`:
- Around line 75-79: The signature for find_sect declares Optional[PESect] but
the implementation raises on miss; make behavior and annotation consistent by
changing find_sect to return None when no section is found instead of
raising—update the method body in the find_sect function to return None
(preserving the loop and comparison against sect.name) and leave the return type
as Optional[PESect]; ensure callers that relied on exceptions handle a None
result where find_sect is used.
---
Nitpick comments:
In `@patcher/Config.py`:
- Around line 52-62: The validation currently rejects any non-absolute
self.build_folder_path in Config.py (the check after setting
self.build_folder_path), which prevents users from supplying relative
build_folder_path even though defaults are resolved; change the logic in the
Config initializer so that if build_folder_path is provided as a relative path
you resolve it against self.target_folder_path (e.g.,
Path(self.target_folder_path) / self.build_folder_path) before running the
is_absolute() check, or alternatively remove the strict absolute check and
document that relative build_folder_path is resolved against target_folder_path;
update places referencing build_folder_path, and keep the symbols
self.build_folder_path and self.target_folder_path consistent.
In `@patcher/Hook.py`:
- Around line 21-28: The nested function replace_address is recreated on every
loop iteration but only reads self._addresses, so move its definition out of the
loop (e.g., make it a private method on the class or define it once above the
loop) and keep the same signature (accepting a re.Match and returning str);
update the code that calls FUNCTION_NAME_RE.subn(replace_address, line) to use
the new standalone method/function so the behavior is unchanged while avoiding
repeated redefinition.
- Line 6: The constant ESCAPE_TRANSLATION is defined but never used (the escape
logic in Hook, referenced by ESCAPE_TRANSLATION, is commented out); remove the
unused constant ESCAPE_TRANSLATION and also remove the now-dead commented escape
logic (or alternatively fully restore and use ESCAPE_TRANSLATION where the
escape was intended) so the file no longer contains unused definitions—look for
ESCAPE_TRANSLATION and the commented escape block in Hook.py and either delete
both or re-enable the escape code to use the constant.
- Line 19: Remove the dead commented-out escape translation by deleting the
commented line containing "line = line.translate(ESCAPE_TRANSLATION)" and then
remove the now-unused ESCAPE_TRANSLATION constant (and any related import or
references) from Hook.py; ensure no other code references ESCAPE_TRANSLATION
before deleting and run tests/linters to confirm there are no remaining unused
symbol errors.
In `@patcher/Patcher.py`:
- Around line 309-310: The deprecation message printed in the loop that iterates
over list_files_at(hooks_folder_path, "**/*.cpp") should be expanded to include
actionable migration guidance; update the print in that loop (inside Patcher.py
where list_files_at and hooks_folder_path are used) to tell users to
rename/convert to .hook files and point them to the relevant docs or conversion
instructions (e.g., add a brief conversion note and a URL or path to
documentation). Ensure the message remains concise and includes the .cpp → .hook
migration step and where to find more details.
- Around line 220-221: Replace the generic Exception raised when the parameter
"template" is not an instance of Template with a TypeError to correctly signal a
type validation failure: in the check that currently reads "if not
isinstance(template, Template): raise Exception(...)" update it to raise
TypeError with a clear message that includes the expected type and the actual
type of template (e.g., "template must be a Template, got <type>"); locate this
check around the Template usage in Patcher.py (the Template class and the
template variable) and make the change there.
- Around line 312-314: The if-statement combines the conditional and the raise
on one line causing formatting/PEP8 issues; modify the block around
run_system(...) so the raise Exception is on its own indented line under the if,
e.g. keep the run_system(...) call as the if condition (referencing run_system,
build_hooks_folder, config.gcc_path, config.asm_flags, hooks_folder_path) and
place a properly indented raise Exception("Errors occurred during building of
hooks files") on the next line.
- Around line 306-307: The if-statement and the raise are jammed on one line; in
the function containing the call that builds generated hooks (the expression
referencing config.gcc_path, config.asm_flags and build_hooks_folder), split the
conditional and the exception into a normal multi-line if block: place the raise
Exception(...) on the next indented line beneath the if so the condition and the
error handling are separated and readable.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.mdconfig_example.jsonmain.pypatcher/Config.pypatcher/Hook.pypatcher/PEData.pypatcher/Patcher.pysection_example.ld
| if __name__ == "__main__": | ||
| start = time.time() | ||
| patcher.patch(*sys.argv) | ||
| patcher.patch(*sys.argv[1:]) |
There was a problem hiding this comment.
Argument unpacking may cause errors with the new single-argument signature.
The patch function now expects a single config_path argument, but *sys.argv[1:] will unpack all CLI arguments. If a user passes extra arguments (as shown in the pipeline failure), this will raise a TypeError.
The pipeline failure shows the old invocation style being used: python main.py "$(pwd)" clang++ ld g++ — this passes 4 arguments to a function expecting 1.
Proposed fix - accept only the first argument
if __name__ == "__main__":
start = time.time()
- patcher.patch(*sys.argv[1:])
+ if len(sys.argv) < 2:
+ print("Usage: python main.py <config_path>")
+ sys.exit(1)
+ patcher.patch(sys.argv[1])
end = time.time()
print(f"Patched in {end-start:.2f}s")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main.py` at line 7, The current invocation uses argument unpacking
patcher.patch(*sys.argv[1:]) which will pass multiple CLI args to patcher.patch
(now expecting a single config_path) and causes a TypeError; change it to pass
only the first CLI argument (e.g., use sys.argv[1] with a sensible default or
validate arg count) so that patcher.patch receives a single config_path string;
update any related error handling to surface a clear message when no argument is
provided.
Comment on lines
17
to
72
| ```json | ||
| { | ||
| // path to target folder. Can be either relative or absolute. | ||
| // if relative path then it will be {config path}/{target_folder_path} | ||
| "target_folder_path": "FA-Binary-Patches", | ||
| // path to build folder. Defaults to "{target_folder_path}/build" | ||
| "build_folder_path": null, | ||
| // paths of input and output files | ||
| "input_exe_path": "ForgedAlliance_base.exe", | ||
| "output_exe_path": "C:\\ProgramData\\FAForever\\bin\\ForgedAlliance_exxt.exe", | ||
| // path to clang++ compiler. Defaults to "clang++" | ||
| "clang": "clang++.exe", | ||
| // path to g++ compiler. Defaults to "g++" | ||
| "gcc": "g++.exe", | ||
| // path to linker. Defaults to "ld" | ||
| "linker": "ld.exe", | ||
| // flags for compilers | ||
| "clang_flags": [ | ||
| "-pipe", | ||
| "-m32", | ||
| "-O3", | ||
| "-nostdlib", | ||
| "-Werror", | ||
| "-masm=intel", | ||
| "-std=c++20", | ||
| "-march=core2" | ||
| ], | ||
| "gcc_flags": [ | ||
| "-pipe", | ||
| "-m32", | ||
| "-Os", | ||
| "-fno-exceptions", | ||
| "-nostdlib", | ||
| "-nostartfiles", | ||
| "-fpermissive", | ||
| "-masm=intel", | ||
| "-std=c++20", | ||
| "-march=core2", | ||
| "-mfpmath=both" | ||
| ], | ||
| "asm_flags": [ | ||
| "-pipe", | ||
| "-m32", | ||
| "-Os", | ||
| "-fno-exceptions", | ||
| "-nostdlib", | ||
| "-nostartfiles", | ||
| "-w", | ||
| "-fpermissive", | ||
| "-masm=intel", | ||
| "-std=c++20", | ||
| "-march=core2", | ||
| "-mfpmath=both" | ||
| ], | ||
| } | ||
| ``` |
There was a problem hiding this comment.
JSON example contains invalid syntax that will cause parse errors.
The JSON example includes // comments and a trailing comma (line 71), both of which are invalid JSON syntax. Users copying this example will encounter parse errors.
Consider either:
- Adding a note that comments must be removed before use
- Using JSONC format indicator (```jsonc) to clarify it's annotated JSON
- Moving comments outside the code block
Suggested fix - use jsonc identifier
-```json
+```jsonc
{
// path to target folder...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 17 - 72, The README's JSON example contains inline //
comments and a trailing comma after the "asm_flags" block which makes it invalid
JSON; update the code fence from ```json to ```jsonc to indicate JSON with
comments and remove the trailing comma after the final "asm_flags" array (or
alternatively move all explanatory comments outside the code block) so consumers
can copy/paste without parse errors—look for the example object containing
"target_folder_path", "clang", "clang_flags", "gcc_flags", and "asm_flags".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
patcher/Patcher.py (3)
220-220: Type hint doesn't match actual usage.The
cwdparameter is typed asOptional[str], but all call sites passPathobjects (e.g.,build_folder_path,build_hooks_folder). Whilesubprocess.runaccepts both, the type hint should reflect actual usage.-def run_process(args, cwd: Optional[str] = None) -> bool: +def run_process(args, cwd: Optional[Path] = None) -> bool:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patcher/Patcher.py` at line 220, The cwd parameter of run_process is annotated as Optional[str] but callers pass pathlib.Path; update the type hint to accept Path as well (e.g., use Union[Path, str] or os.PathLike[str]) and add the necessary import (from pathlib import Path or import os and os.PathLike) so the annotation matches actual usage in run_process and its call sites like build_folder_path and build_hooks_folder.
6-13: Remove unused imports.
json(line 6) andTemplatefromstring.templatelib(line 12) are imported but never used in this file. TheConfigclass handles JSON loading internally, andTemplateappears to be a leftover from a previous implementation.🧹 Remove unused imports
-import json from typing import Optional import struct import itertools from patcher import Hook from .Config import Config -from string.templatelib import Template import subprocess🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patcher/Patcher.py` around lines 6 - 13, The file imports unused symbols: remove the top-level imports of json and Template (from string.templatelib) in Patcher.py; keep the remaining imports (Optional, struct, itertools, Hook, Config, subprocess) and verify there are no references to json or Template elsewhere in Patcher.py before committing the change (search for any usage in functions or classes such as any code interacting with Config or Hook).
220-222: Return value semantics are confusing.
run_processreturnsTrueon failure andFalseon success, which is counterintuitive. While the callers handle this correctly, the semantics are easy to misuse.Consider either renaming to clarify failure indication, or inverting to return success status:
Option A: Rename to clarify intent
-def run_process(args, cwd: Optional[str] = None) -> bool: +def run_process_failed(args, cwd: Optional[str] = None) -> bool: + """Returns True if the process failed (non-zero exit code).""" result = subprocess.run(args, cwd=cwd) return result.returncode != 0Option B: Return success and invert callers
-def run_process(args, cwd: Optional[str] = None) -> bool: +def run_process(args, cwd: Optional[str] = None) -> bool: + """Returns True if the process succeeded.""" result = subprocess.run(args, cwd=cwd) - return result.returncode != 0 + return result.returncode == 0 # Then update callers: -if run_process(...): +if not run_process(...): raise Exception(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patcher/Patcher.py` around lines 220 - 222, The function run_process currently returns True for failure which is counterintuitive; change run_process to return True on success by returning result.returncode == 0 (instead of result.returncode != 0) and update every caller that relies on the current inverted semantics (invert their conditionals or boolean checks) so they treat a True return as success; ensure references to run_process in code paths are adjusted to preserve behavior (e.g., flip if/else branches or negations) and run tests to confirm no logic was reversed incorrectly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patcher/Patcher.py`:
- Around line 308-311: The invocation of run_process using (config.gcc_path,
"-c", *config.asm_flags, hooks_folder_path / "*.cpp") fails because the glob
"*.cpp" isn't expanded; change this to collect the actual .cpp files under
hooks_folder_path (e.g., use hooks_folder_path.glob("*.cpp") or similar), build
a list of file paths (skip if empty) and pass that list combined with
config.gcc_path and flags into run_process (preserve cwd=build_hooks_folder);
reference run_process, config.gcc_path, hooks_folder_path, and
build_hooks_folder when locating where to apply the change and optionally make
the whole block conditional if no .cpp hook files are expected.
- Around line 299-303: The command invoking run_process is passing the literal
glob pattern build_hooks_folder / "*.cpp" which won't be expanded because
run_process uses subprocess with shell=False; fix by expanding the glob before
calling run_process (e.g., use pathlib.Path.glob or glob.glob on
build_hooks_folder / "*.cpp") and build a list of source file paths to append to
the argv (use config.gcc_path and config.asm_flags as the first elements), and
handle the case of an empty list (raise or no-op) so run_process receives actual
file paths rather than the "*.cpp" string.
---
Nitpick comments:
In `@patcher/Patcher.py`:
- Line 220: The cwd parameter of run_process is annotated as Optional[str] but
callers pass pathlib.Path; update the type hint to accept Path as well (e.g.,
use Union[Path, str] or os.PathLike[str]) and add the necessary import (from
pathlib import Path or import os and os.PathLike) so the annotation matches
actual usage in run_process and its call sites like build_folder_path and
build_hooks_folder.
- Around line 6-13: The file imports unused symbols: remove the top-level
imports of json and Template (from string.templatelib) in Patcher.py; keep the
remaining imports (Optional, struct, itertools, Hook, Config, subprocess) and
verify there are no references to json or Template elsewhere in Patcher.py
before committing the change (search for any usage in functions or classes such
as any code interacting with Config or Hook).
- Around line 220-222: The function run_process currently returns True for
failure which is counterintuitive; change run_process to return True on success
by returning result.returncode == 0 (instead of result.returncode != 0) and
update every caller that relies on the current inverted semantics (invert their
conditionals or boolean checks) so they treat a True return as success; ensure
references to run_process in code paths are adjusted to preserve behavior (e.g.,
flip if/else branches or negations) and run tests to confirm no logic was
reversed incorrectly.
Comment on lines
+299
to
+303
| if run_process(( | ||
| config.gcc_path, "-c", *config.asm_flags, build_hooks_folder / "*.cpp" | ||
| ), cwd=build_hooks_folder): | ||
| raise Exception( | ||
| "Errors occurred during building of generated hooks files") |
There was a problem hiding this comment.
Glob patterns won't expand without shell.
subprocess.run with shell=False (the default) does not perform glob expansion. Passing build_hooks_folder / "*.cpp" will literally pass the string "*.cpp" to gcc, which will fail to find files.
You need to expand the glob manually:
🐛 Proposed fix using glob expansion
+import glob
- if run_process((
- config.gcc_path, "-c", *config.asm_flags, build_hooks_folder / "*.cpp"
- ), cwd=build_hooks_folder):
+ cpp_files = list(build_hooks_folder.glob("*.cpp"))
+ if cpp_files and run_process((
+ config.gcc_path, "-c", *config.asm_flags, *cpp_files
+ ), cwd=build_hooks_folder):
raise Exception(
"Errors occurred during building of generated hooks files")📝 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
| if run_process(( | |
| config.gcc_path, "-c", *config.asm_flags, build_hooks_folder / "*.cpp" | |
| ), cwd=build_hooks_folder): | |
| raise Exception( | |
| "Errors occurred during building of generated hooks files") | |
| cpp_files = list(build_hooks_folder.glob("*.cpp")) | |
| if cpp_files and run_process(( | |
| config.gcc_path, "-c", *config.asm_flags, *cpp_files | |
| ), cwd=build_hooks_folder): | |
| raise Exception( | |
| "Errors occurred during building of generated hooks files") |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 302-303: Create your own exception
(TRY002)
[warning] 302-303: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patcher/Patcher.py` around lines 299 - 303, The command invoking run_process
is passing the literal glob pattern build_hooks_folder / "*.cpp" which won't be
expanded because run_process uses subprocess with shell=False; fix by expanding
the glob before calling run_process (e.g., use pathlib.Path.glob or glob.glob on
build_hooks_folder / "*.cpp") and build a list of source file paths to append to
the argv (use config.gcc_path and config.asm_flags as the first elements), and
handle the case of an empty list (raise or no-op) so run_process receives actual
file paths rather than the "*.cpp" string.
Comment on lines
+308
to
311
| if run_process( | ||
| (config.gcc_path, "-c", *config.asm_flags, hooks_folder_path / "*.cpp"), | ||
| cwd=build_hooks_folder): | ||
| raise Exception("Errors occurred during building of hooks files") |
There was a problem hiding this comment.
Same glob expansion issue for hooks folder.
The wildcard in hooks_folder_path / "*.cpp" won't expand. Apply the same fix pattern here.
🐛 Proposed fix
- if run_process(
- (config.gcc_path, "-c", *config.asm_flags, hooks_folder_path / "*.cpp"),
- cwd=build_hooks_folder):
+ hook_cpp_files = list(hooks_folder_path.glob("*.cpp"))
+ if hook_cpp_files and run_process(
+ (config.gcc_path, "-c", *config.asm_flags, *hook_cpp_files),
+ cwd=build_hooks_folder):
raise Exception("Errors occurred during building of hooks files")Note: The deprecation warning at lines 305-306 suggests .cpp hooks are being phased out. If no .cpp files are expected, consider removing this block entirely or making it conditional.
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 311-311: Create your own exception
(TRY002)
[warning] 311-311: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patcher/Patcher.py` around lines 308 - 311, The invocation of run_process
using (config.gcc_path, "-c", *config.asm_flags, hooks_folder_path / "*.cpp")
fails because the glob "*.cpp" isn't expanded; change this to collect the actual
.cpp files under hooks_folder_path (e.g., use hooks_folder_path.glob("*.cpp") or
similar), build a list of file paths (skip if empty) and pass that list combined
with config.gcc_path and flags into run_process (preserve
cwd=build_hooks_folder); reference run_process, config.gcc_path,
hooks_folder_path, and build_hooks_folder when locating where to apply the
change and optionally make the whole block conditional if no .cpp hook files are
expected.
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.
This is a major patcher change that refines patching process and includes new features:
.jsonconfig for patching game executable with adjustable paths and flagsdefines.hfilesection.ldfile was changed to contain all info needed for linker, patcher just appends addresses.hooksare refined: these can be placed in sections folder so hook is near its respective .cxx/.cpp fileoffset, likepush @OBSTRUCTSBUILDING->push offset @OBSTRUCTSBUILDING*.cpphooks are deprecated and prohibited to be used in further patches, use.hookSummary by CodeRabbit
New Features
Documentation
Versions