Pass patcher related options via config #11
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a Config dataclass and example JSON; refactors patcher to load configuration and build Template-based commands; run_system now accepts a Template; PEData.find_sect now raises when missing; main forwards argv[1:]; CI workflow updated to run the config-driven flow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main as "main.py"
participant Config as "patcher/Config.py"
participant Patcher as "patcher/Patcher.py"
participant FS as "Filesystem"
participant Tool as "Compiler/Linker"
rect rgba(200,200,255,0.5)
User->>Main: run script with config path
Main->>Patcher: call patch(config_path)
end
rect rgba(200,255,200,0.5)
Patcher->>Config: Config.load_from_json(path)
Config-->>Patcher: Config(instance with paths, flags, functions)
end
rect rgba(255,230,200,0.5)
Patcher->>FS: read input binary, hooks, headers
Patcher->>FS: write build artifacts (section.ld, patch.pe, defines)
FS-->>Patcher: artifact paths
end
rect rgba(255,200,200,0.5)
Patcher->>Tool: run_system(template) — compile/link via Template
Tool-->>Patcher: exit status, outputs
end
rect rgba(230,255,255,0.5)
Patcher->>FS: write final output to config.output_path
Patcher-->>User: completion / errors
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Fix all issues with AI agents
In `@patcher/Config.py`:
- Around line 24-33: In load_from_json (the Config classmethod) resolve the
incoming Path before using it to derive defaults so relative config paths won't
produce a relative target_folder_path that fails __post_init__; i.e.,
convert/resolve the path to an absolute Path at the start of load_from_json,
then open that resolved path and use resolved_path.parent as the default for
target_folder_path while keeping the rest of the keys the same.
In `@patcher/Patcher.py`:
- Around line 331-335: The shell command passed to run_system is building an
unquoted string using config.clang_path, the joined config.clang_flags,
include_folder_path, section_folder_path / "main.cxx", and build_folder_path /
"clangfile.o" which will break on paths containing spaces; update the run_system
invocations (the one shown and the other occurrences noted around the file) to
safely quote or shell-escape each path and each flag before joining (e.g., wrap
config.clang_path, each element from config.clang_flags, include_folder_path,
the section_folder_path/"main.cxx" token, and the
build_folder_path/"clangfile.o" token in quotes or use a function to
shell-escape them) so the command string passed to run_system treats each
path/flag as a single argument.
- Around line 467-468: The format spec in the exception message uses a stray
space in the hex formatter; replace the format expression f"0x{exxt_sect.f_size:
x}" with a proper hex specifier (e.g. f"0x{exxt_sect.f_size:x}") in the raise
Exception call so the output reads like 0x80000 (reference: exxt_sect.f_size in
the raise Exception block).
🧹 Nitpick comments (2)
patcher/Config.py (1)
18-20: Flag type hints don’t match JSON shape.
tuple[str]is a fixed‑length 1‑tuple, while JSON loads lists. Considertuple[str, ...]orSequence[str], and normalize to tuples in__post_init__.♻️ Proposed refactor
-from typing import Self +from typing import Self, Sequence ... - clang_flags: tuple[str] = () - gcc_flags: tuple[str] = () - asm_flags: tuple[str] = () + clang_flags: Sequence[str] = () + gcc_flags: Sequence[str] = () + asm_flags: Sequence[str] = () ... def __post_init__(self): self.target_folder_path = Path(self.target_folder_path) self.build_folder_path = Path(self.build_folder_path)\ if self.build_folder_path \ else self.target_folder_path / "build" self.clang_path = Path(self.clang_path) self.gcc_path = Path(self.gcc_path) self.linker_path = Path(self.linker_path) + + self.clang_flags = tuple(self.clang_flags) + self.gcc_flags = tuple(self.gcc_flags) + self.asm_flags = tuple(self.asm_flags)Also applies to: 42-50
patcher/PEData.py (1)
75-79: Alignfind_sect's contract with its new behavior.The return type
Optional[PESect]is misleading—the method always raisesExceptionand never returnsNone. Call sites inPatcher.py(lines 450, 456) don't check forNoneand directly use the result, relying on exception handling.Consider a dedicated exception (e.g.,
SectionNotFoundError) and a non-optional return type (PESect) to clarify the actual contract.♻️ Proposed refactor
+class SectionNotFoundError(LookupError): + pass + ... -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}") + raise SectionNotFoundError(f"Couldn't find section {name}")
|
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@patcher/Config.py`:
- Around line 52-62: The build_folder_path value is converted to a Path but not
resolved to an absolute path like target_folder_path, causing the is_absolute()
check to fail for relative inputs; update the Config initialization so after
converting self.build_folder_path to a Path, if it is not absolute then join it
with self.target_folder_path (e.g., self.target_folder_path /
self.build_folder_path) and resolve it to an absolute Path before running the
existing validation; reference the build_folder_path and target_folder_path
attributes in Config to locate where to perform this normalization.
In `@patcher/Patcher.py`:
- Around line 290-306: The run_system function currently only quotes Path values
with spaces and concatenates parts unsafely, so replace the ad-hoc quoting with
proper shell escaping: import shlex and, inside run_system (function name) when
iterating template parts (loop over part and using part.value), call
shlex.quote(str(value)) for non-string parts instead of the current Path/space
check or manual f"\"{value}\"" quoting; also build the final command by joining
the command list with a space (e.g., " ".join(command)) and keep the existing
newline replacement before calling os.system(command.replace("\n", " ")).
- Line 12: The README's Versions section is out of date: the code imports
string.templatelib (which requires Python 3.14/PEP 750), so update the README
Versions entry that currently says "python: 3.12.4" to "python: 3.14" to match
the CI and the import usage; search for the README Versions block and replace
the Python version string accordingly so documentation aligns with the use of
string.templatelib in Patcher.py.
🧹 Nitpick comments (4)
main.py (1)
7-7: Argument unpacking mismatch withpatch()signature.
patch()expects a singleconfig_pathargument, but*sys.argv[1:]unpacks all CLI arguments. If the user provides zero or more than one argument, this raises aTypeErrorwith a confusing message. Consider passing the argument directly and validating input.♻️ Suggested improvement
if __name__ == "__main__": + if len(sys.argv) != 2: + print(f"Usage: {sys.argv[0]} <config_path>") + sys.exit(1) start = time.time() - patcher.patch(*sys.argv[1:]) + patcher.patch(sys.argv[1]) end = time.time() print(f"Patched in {end-start:.2f}s")config_example.json (1)
45-57: Inconsistent hex address formatting.Some addresses have leading zeros (
"0x00A89950","0x00A848E8") while others don't ("0xA8211E"). This inconsistency doesn't affect functionality but could indicate copy-paste errors. Consider normalizing the format.♻️ Suggested normalization
- "__CxxThrowException@8": "0x00A89950", + "__CxxThrowException@8": "0xA89950", "_memset": "0xA89110", - "__invoke_watson": "0x00A848E8" + "__invoke_watson": "0xA848E8"patcher/Config.py (1)
19-21: Type annotations don't match actual values from JSON.The annotations specify
tuple[str]butjson.load()returns lists, and there's no conversion. This works at runtime but may confuse type checkers.♻️ Consider using `list[str]` or converting to tuples
Either change the type hints:
- clang_flags: tuple[str] = () - gcc_flags: tuple[str] = () - asm_flags: tuple[str] = () + clang_flags: list[str] = field(default_factory=list) + gcc_flags: list[str] = field(default_factory=list) + asm_flags: list[str] = field(default_factory=list)Or convert in
load_from_json:- clang_flags=config.get("clang_flags", Config.clang_flags), + clang_flags=tuple(config.get("clang_flags", Config.clang_flags)),patcher/Patcher.py (1)
352-355: Unusual indentation onraisestatements.The
raise Exception(...)statements are indented more deeply than the surrounding code. While syntactically valid, this harms readability and looks like a formatting error.♻️ Consistent indentation
if run_system( t""" cd {build_folder_path} & {config.gcc_path} {" ".join(config.gcc_flags)} -I {include_folder_path} -Wl,-T,section.ld,--image-base,{image_base},-s,-Map,sectmap.txt,-o,section.pe {section_folder_path / "main.cpp"}"""): - raise Exception("Errors occurred during building of patch files") + raise Exception("Errors occurred during building of patch files")Apply similar fix to lines 384 and 444.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Around line 71-72: The workflow fails because the repo is checked out to the
workspace root but the job later runs the command sequence "mkdir
FA-Binary-Patches/build" followed by "cd fa-python-binary-patcher & python
main.py config_example.json" which expects a fa-python-binary-patcher directory;
fix by either updating the checkout step to set path: fa-python-binary-patcher
so that "cd fa-python-binary-patcher" will succeed, or remove the "cd
fa-python-binary-patcher" and run "python main.py config_example.json" from the
workspace root after "mkdir FA-Binary-Patches/build" so the commands run in the
correct location.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@patcher/Patcher.py`:
- Around line 312-314: The command interpolation is joining flag lists into a
single string which becomes one quoted argument; update run_system to detect
list/tuple values and shlex.quote each element individually (joining them with
spaces) so flags are passed as separate arguments, then change the call sites
that currently do " ".join(config.clang_flags) (and other flag joins used around
the build commands in Patcher.py — e.g., the occurrences near the current uses
of config.clang_flags at the compile lines and the other flag interpolations
referenced) to pass the raw list/tuple (config.clang_flags, etc.) instead of a
pre-joined string so run_system can handle proper quoting per-element.
🧹 Nitpick comments (3)
patcher/Patcher.py (3)
6-6: Unusedjsonimport.The
jsonmodule is imported but not used in this file. The JSON loading appears to be handled withinConfig.load_from_json.🧹 Suggested fix
-import json
275-276: Add type hint forconfig_pathparameter.The function signature lacks a type annotation. Since it's passed to
Path(), adding a type hint improves clarity and enables better static analysis.✨ Suggested fix
-def patch(config_path): +def patch(config_path: str | Path):
318-321: Excessive whitespace in command template.The multi-line template string preserves indentation spaces, resulting in commands like
gcc -c -I ...in logs. While not functionally broken, it makes debugging harder.✨ Suggested fix - use line continuation or dedicated formatting
if run_system( - t""" cd {build_folder_path} & - {config.gcc_path} {" ".join(config.gcc_flags)} -I {include_folder_path} -Wl,-T,section.ld,--image-base,{image_base},-s,-Map,sectmap.txt,-o,section.pe {section_folder_path / "main.cpp"}"""): + t"""cd {build_folder_path} & {config.gcc_path} {config.gcc_flags} -I {include_folder_path} -Wl,-T,section.ld,--image-base,{image_base},-s,-Map,sectmap.txt,-o,section.pe {section_folder_path / "main.cpp"}"""): raise Exception("Errors occurred during building of patch files")
|
Can we remove this code rabbit thing completely? It is annoying and doesn't provide any useful tips... |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@patcher/Patcher.py`:
- Around line 258-273: In run_system, non-Path interpolated template parts
(variable value from `part.value`) may be integers and are appended directly to
`command`, causing `"".join(command)` to raise a TypeError; convert any
non-Path, non-str values to strings before appending. Locate the loop over
`template` in `run_system` (inspect `part`, `value`, and `command`) and ensure
branches: if `isinstance(value, Path)` use shlex.quote(str(value)); elif
`isinstance(value, str)` append as-is; else append `str(value)` so all entries
in `command` are strings before joining.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.