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 |
Ran a focused audit looking for the next bugs after today's 9 fixes. Shipping 6 of the 10 findings; the other 4 are lower-impact or require larger restructuring. #1 — Steer-race between handleWatch and alarmInner [HIGH] The DO is single-threaded but `await` yields. alarmInner does fetch() on Replicas /history which can yield for 100s of ms — during that window a steer /watch races the alarm's read-modify-write on lines[]. Whoever writes second clobbers the other's in-memory copy. Wrapped the steer's state mutation in this.state.blockConcurrencyWhile() so the alarm's in-flight body completes before lines[] is touched. #7 — setTerminal partial-execution leaves un-swapped reaction [MEDIUM] Even with the atomic phase+pendingCleanup write at the top, if renderAndSend or unpinStatus threw, swapReaction never ran — leaving the user's 👀 ack reaction forever (no 🎉/😭). Wrapped each step in its own try/catch so they run independently and the rest of the cleanup always fires. #4 — verifyTurnDelivered didn't actually recover missing reply [HIGH] The docstring promised "if replyEventId missing → re-call sendReplyAsOwnBlock" but the code just logged. Now stashes lastResultHtml in storage during setTerminal so verify can actually re-send via the stable txn id (Matrix dedupes if the prior send did land but our /event/{id} lookup raced). #8 — recover-room mention check used encrypted ev.content [MEDIUM] shouldHandleMessage reads m.mentions + formatted_body from content, but the OUTER ev.content is the encrypted wrapper (algorithm / ciphertext / session_id). Mention detection silently failed in multi-user encrypted rooms during recovery dispatch. Fixed to pass the DECRYPTED inner.content. #6 — lines[] unbounded growth on thinking-heavy turns [MEDIUM] Long planning sessions push hundreds of 💬 narration lines that bloat the serialized state on every ~180ms alarm tick. Capped at 300 entries per segment by dropping oldest non-tool lines first (preserves 🔄/✅/❌ tool entries + 💬 You: steer markers). toolLineIndex pointers are shifted in lockstep when an entry before them is dropped. #2 — key-request:* DO storage grew unbounded forever [HIGH-silent] maybeSendKeyRequest stored `key-request:{room}:{session}` → ts to dedupe inside a 5-min window but never deleted. Every Megolm session rotation accumulated a permanent entry. Bucketed the dedupKey by 5-min window AND added a piggy-back cleanup that prunes entries older than 2 buckets on each call. Bounded storage. Deferred (with rationale): #3 pending-decrypt prune needs a cron sweeper; #5 KV dedupe race needs deeper redesign; #9 cachedKeys stale is low-impact (manual reload endpoint suffices); #10 parsePlan hijack has rare false-positives that haven't been observed. Tests: 67/67 pass; typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: itsablabla <itsablabla@users.noreply.github.com>
1 participant
Added from Capy skills marketplace:
brightdata/skillsskillssh/skillsskillssh/skillsbrowser-use/browser-useanthropics/skillsanthropics/skillsanthropics/skillsanthropics/skillsanthropics/skills