Add 9 skills from marketplace#9
Conversation
…reator, pdf, docx, pptx, xlsx
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| except subprocess.TimeoutExpired: | ||
| return ( | ||
| None, | ||
| f"Successfully accepted all tracked changes: {input_file} -> {output_file}", |
There was a problem hiding this comment.
WARNING: TimeoutExpired is caught and treated as success
When the 30-second timeout fires, LibreOffice has not finished writing the file. The output DOCX will likely be incomplete or corrupt. Returning the success message here is incorrect — a timeout means the operation did not complete.
The TimeoutExpired branch should return an error, not a success message:
| f"Successfully accepted all tracked changes: {input_file} -> {output_file}", | |
| None, | |
| f"Error: LibreOffice timed out accepting changes for: {input_file}", |
| date=ts, | ||
| initials=initials, | ||
| para_id=para_id, | ||
| text=text, |
There was a problem hiding this comment.
WARNING: Unsanitized text is interpolated directly into an XML template
The COMMENT_XML template uses Python string .format() to insert text verbatim. If the caller passes a string containing <, >, &, ", or ' characters (common in document comments), the resulting <w:t> element will contain malformed XML and _append_xml will fail or silently produce a corrupt document.
The docstring says text must be "pre-escaped XML," but this is not enforced programmatically. At minimum, apply xml.sax.saxutils.escape(text) before formatting, or use DOM APIs to set the text node value (which handle escaping automatically).
| for field in fields_data["form_fields"]: | ||
| page_num = field["page_number"] | ||
|
|
||
| page_info = next(p for p in fields_data["pages"] if p["page_number"] == page_num) |
There was a problem hiding this comment.
CRITICAL: Bare next() call will raise StopIteration if the page is not found
next(p for p in fields_data["pages"] if p["page_number"] == page_num) raises StopIteration (which propagates as an unhandled exception) when no entry in pages matches page_num. This crashes the script entirely rather than providing a useful error message.
Add a default or handle the case explicitly:
| page_info = next(p for p in fields_data["pages"] if p["page_number"] == page_num) | |
| page_info = next((p for p in fields_data["pages"] if p["page_number"] == page_num), None) |
Then add a guard after this line:
if page_info is None:
print(f"Error: no page info found for page {page_num}")
sys.exit(1)| args = parser.parse_args() | ||
|
|
||
| path = Path(args.path) | ||
| assert path.exists(), f"Error: {path} does not exist" |
There was a problem hiding this comment.
WARNING: assert used for user-facing input validation
assert statements are stripped when Python runs with the -O (optimize) flag (python -O validate.py ...), silently skipping these checks. For a CLI tool, use explicit conditionals with print + sys.exit(1) instead:
if not path.exists():
print(f"Error: {path} does not exist")
sys.exit(1)The same applies to the assert calls on lines 61–68 and 77.
|
|
||
|
|
||
| def _ensure_shim() -> Path: | ||
| if _SHIM_SO.exists(): |
There was a problem hiding this comment.
WARNING: Shared /tmp path for compiled .so creates a TOCTOU race condition
_SHIM_SO is hardcoded to /tmp/lo_socket_shim.so. On a multi-user system, a malicious local user can pre-create or replace this path with arbitrary code before or after the existence check on this line. The check if _SHIM_SO.exists(): return _SHIM_SO does not verify ownership or integrity of the existing file.
Consider namespacing the path with the effective UID (e.g., Path(tempfile.gettempdir()) / f"lo_socket_shim_{os.getuid()}.so"), or use tempfile.mkstemp() to create the file atomically.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)
Files Reviewed (22 files)
Fix these issues in Kilo Cloud Reviewed by claude-4.6-sonnet-20260217 · 1,293,285 tokens |
1 participant
Added from Capy skills marketplace:
brightdata/skillsskillssh/skillsskillssh/skillsbrowser-use/browser-useanthropics/skillsanthropics/skillsanthropics/skillsanthropics/skillsanthropics/skills