Skip to content

fix: resolve lint and type errors - #2

Merged
rishitank merged 3 commits into
mainfrom
fix/lint-and-type-errors
Feb 3, 2026
Merged

rishitank merged 3 commits into
mainfrom
fix/lint-and-type-errors

Conversation

@rishitank

Copy link
Copy Markdown
Owner

Summary

This PR fixes all lint (ruff) and type (mypy) errors in the codebase to unblock CI.

Changes

browser.py

  • Changed from typing import AsyncGenerator to from collections.abc import AsyncGenerator
  • Added proper type annotations to __init__, start, stop, _perform_action methods
  • Added Playwright type import
  • Fixed dict type hints to dict[str, Any] and dict[str, int]
  • Added null checks for self._browser before use
  • Changed video_size usage to explicit {"width": ..., "height": ...} dicts

config.py

  • Changed def video_size(self) -> dict: to def video_size(self) -> dict[str, int]:

vision.py

  • Added contextlib, time imports
  • Added type: ignore[attr-defined] for google.generativeai methods that lack type stubs
  • Added proper return type annotations to __init__ methods
  • Changed try/except/pass to with contextlib.suppress(Exception)
  • Fixed raise ... from err pattern for ImportError
  • Added Any type annotations for dynamic objects

server.py

  • Added contextlib and Literal imports
  • Fixed import sorting
  • Changed ctx: Context[...] = None to ctx: Context[...] | None = None for all tool functions
  • Added null checks for ctx at start of each tool function
  • Changed try/except/pass to with contextlib.suppress(Exception)
  • Added return type annotation to main() function
  • Fixed transport type to use Literal["stdio", "streamable-http"]

tests/test_config.py

  • Removed trailing whitespace on line 33

Testing

  • ✅ uv run ruff check src/ tests/ - All checks passed
  • ✅ uv run mypy src/ - Success: no issues found in 5 source files
  • ✅ uv run pytest tests/ -v - 3 passed

Pull Request opened by Augment Code with guidance from the PR author

- browser.py: Fix AsyncGenerator import, add type annotations, add null checks
- config.py: Add type parameter to video_size return type
- vision.py: Add type annotations, fix type ignores for untyped google.generativeai
- server.py: Fix ctx parameter types, add null checks, add return types
- tests/test_config.py: Remove trailing whitespace
@coderabbitai

coderabbitai Bot commented Feb 1, 2026 •

Copy link
Copy Markdown

Summary by CodeRabbit

  • New Features

    • Improved recording flows: screenshots and recordings reliably saved and exposed to callers.
  • Improvements

    • Stronger lifecycle and resource management for server and browser processes.
    • Safer runtime checks to prevent uninitialised usage.
    • Cleaner typing and signatures for clearer behaviour.
  • Bug Fixes

    • More robust cleanup (suppresses file-delete errors) and video finalisation.
  • Style

    • Minor formatting and whitespace tidy-ups.

Walkthrough

The PR applies whitespace fixes, adds type annotations across modules, introduces AppContext and app_lifespan for server lifecycle, adds a recording_context async context manager to BrowserRecorder, strengthens runtime guards and cleanup, and updates several function signatures and return type annotations.

Changes

Cohort / File(s) Summary
Whitespace & Tests
src/animawatch/__init__.py, tests/__init__.py, tests/test_config.py
Removed trailing/extra blank lines and minor formatting tweaks.
Configuration Types
src/animawatch/config.py
Added ViewportSize TypedDict and changed Settings.video_size() to return ViewportSize.
Browser recorder
src/animawatch/browser.py
Added type annotations for Playwright/browser, recording_context async context manager, stronger lifecycle guards (start/stop), updated action/screenshot/record signatures and return types, temp file handling and explicit video saving.
Server lifecycle & DI
src/animawatch/server.py
Added AppContext dataclass and app_lifespan asynccontextmanager; updated tool function signatures to accept optional ctx and raise at runtime if absent; improved cleanup using contextlib.suppress; refined typing (Literal).
Vision providers
src/animawatch/vision.py
Added type annotations, explicit Any-typing for provider internals, adjusted init signatures, response handling and cleanup using contextlib.suppress; lazy import/error chaining for Ollama.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

Apprentice, behold the code refined,
Types aligned, stray blanks confined,
Lifecycles bound and guards deployed,
The power of the Dark Side—typed, not void. ⚡

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately captures the main objective: fixing lint and type errors across the codebase to unblock CI.
Description check ✅ Passed The description clearly and comprehensively explains the changes made to address lint and type errors, organised by file with specific details about each modification.
Docstring Coverage ✅ Passed Docstring coverage is 86.36% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/lint-and-type-errors

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai Bot previously requested changes Feb 1, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/animawatch/browser.py (1)

135-136: ⚠️ Potential issue | 🟡 Minor

I find your lack of secure temporary file handling... disturbing, Apprentice. tempfile.mktemp harbours the weakness of deprecated code.

The mktemp function has been deprecated since Python 2.3 due to inherent security concerns—a race condition exists between name generation and file creation. Use tempfile.mkstemp or tempfile.NamedTemporaryFile with delete=False instead to harness the power of the Dark Side.

🛡️ Proposed fix using mkstemp
+            import os
             screenshot_path = Path(tempfile.mktemp(suffix=".png"))
-            await page.screenshot(path=str(screenshot_path), full_page=full_page)
+            fd, tmp_path = tempfile.mkstemp(suffix=".png")
+            os.close(fd)
+            screenshot_path = Path(tmp_path)
+            await page.screenshot(path=str(screenshot_path), full_page=full_page)

Note: This requires adding import os at the top of the file (currently missing).

🤖 Fix all issues with AI agents
In `@src/animawatch/browser.py`:
- Around line 124-129: The code duplicates viewport construction by passing
viewport={"width": settings.video_width, "height": settings.video_height} to
self._browser.new_context; replace that dict with the canonical property
(settings.video_size or settings.video_size()) used elsewhere to avoid
duplication and ensure consistency—locate where new_context is called in
animawatch.browser (references: self._browser, new_context) and change the
viewport argument to use settings.video_size (or the project's video_size
property accessor) instead of separate video_width/video_height fields.
- Around line 59-65: The null-check after awaiting self.start() is correct and
should remain to guard against a missing _browser; replace the duplicated inline
viewport and record_video_size dictionaries with the shared settings.video_size
property for consistency and to avoid drift: in the code that calls await
self.start() and then uses self._browser.new_context(...), change
viewport={"width": settings.video_width, "height": settings.video_height} and
record_video_size={"width": settings.video_width, "height":
settings.video_height} to use viewport=settings.video_size and
record_video_size=settings.video_size (references: self.start(), self._browser,
new_context, settings.video_size).

In `@src/animawatch/server.py`:
- Line 25: Remove the unnecessary noqa suppression on the import line for
VisionProvider and get_vision_provider: delete " # noqa: F401" from the import
statement (the symbol VisionProvider is actually used to annotate the vision
field on the AppContext dataclass), so keep the import but remove the trailing
noqa and ensure the import remains as "from .vision import VisionProvider,
get_vision_provider".

In `@src/animawatch/vision.py`:
- Around line 57-58: The cleanup currently suppresses all exceptions using
contextlib.suppress(Exception); narrow this to only filesystem-related errors by
replacing contextlib.suppress(Exception) with contextlib.suppress(OSError) (or a
more specific exception like FileNotFoundError/PermissionError if appropriate)
around the genai.delete_file(video_file.name) call so only expected
file-deletion failures are ignored while other exceptions still surface; refer
to the contextlib.suppress usage, genai.delete_file, and video_file.name to
locate and update the code.
- Around line 46-48: The loop using time.sleep blocks the event loop—replace
synchronous time.sleep(1) with an awaited asyncio.sleep(1) inside the async
routine that polls video_file (the loop that checks video_file.state.name ==
"PROCESSING" and calls genai.get_file(video_file.name)); update the call to
await asyncio.sleep(1) and ensure the function is async so awaiting is valid,
then remove the unused time import if nothing else uses it.

Comment thread src/animawatch/browser.py Outdated
Comment thread src/animawatch/browser.py Outdated
Comment thread src/animawatch/server.py Outdated
Comment thread src/animawatch/vision.py
Comment thread src/animawatch/vision.py Outdated
- browser.py: Replace tempfile.mktemp with mkstemp (security fix)
- browser.py: Use settings.video_size instead of duplicated dicts
- vision.py: Use asyncio.sleep instead of blocking time.sleep
- vision.py: Narrow exception suppression from Exception to OSError
- server.py: Remove unnecessary noqa comment
- server.py: Narrow exception suppression from Exception to OSError
- config.py: Use TypedDict for Playwright ViewportSize compatibility
@rishitank

Copy link
Copy Markdown
Owner Author

@coderabbitai resolve

All requested changes have been implemented:

  1. ✅ tempfile.mktemp → tempfile.mkstemp (security fix)
  2. ✅ Added import os for file descriptor handling
  3. ✅ Replaced duplicated viewport dicts with settings.video_size
  4. ✅ time.sleep → asyncio.sleep (non-blocking)
  5. ✅ contextlib.suppress(Exception) → contextlib.suppress(OSError) (narrowed scope)
  6. ✅ Removed unnecessary # noqa: F401 comment
  7. ✅ Added ViewportSize TypedDict for Playwright type compatibility

Please re-review.

@rishitank
rishitank dismissed coderabbitai[bot]’s stale review February 2, 2026 10:24

All requested changes have been addressed in commit fed876d. Security issues fixed (mktemp→mkstemp), async sleep, narrowed exception handling, removed code duplication.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
src/animawatch/vision.py (2)

79-113: ⚠️ Potential issue | 🟠 Major

Eliminate Any types by importing Ollama's native Pydantic models, Apprentice.

The code violates the no-Any directive by relying on untyped annotations. The ollama library already provides ChatResponse (a Pydantic model), making your custom TypedDict unnecessary. Import and use Ollama's models directly—this is the power of proper type safety.

🧬 Correct typing using Ollama's Pydantic models
+from ollama import AsyncClient, ChatResponse

             import ollama

-            self.client: Any = ollama.AsyncClient(host=settings.ollama_host)
+            self.client: AsyncClient = AsyncClient(host=settings.ollama_host)

-        response: dict[str, Any] = await self.client.chat(
+        response: ChatResponse = await self.client.chat(

No custom TypedDict needed—ChatResponse.message.content provides the structure you require.

Per coding guidelines: "Type hints everywhere (no Any unless justified)" and "Pydantic for data validation". The ollama library provides these types; use them.


32-73: ⚠️ Potential issue | 🔴 Critical

Your reliance on deprecated rebel scum and manufactured Protocols reveals ignorance of the official typing force, Apprentice.

The google-generativeai SDK reached end-of-support on 30 November 2025—your code now runs without a safety net. More troubling: official types already exist within the package itself. upload_file() returns file_types.File, and GenerativeModel.generate_content() returns generation_types.GenerateContentResponse—yet you hide behind Any and # type: ignore.

Rather than crafting custom Protocols, migrate to Google's current google-genai SDK immediately. The old library is dead; do not waste effort patching its corpse with Protocols.

Should you refuse migration and remain bound to the deprecated package, eliminate the Any rebel scum by importing and using the official types:

from google.generativeai import file_types, generation_types

self.model: genai.GenerativeModel = genai.GenerativeModel(settings.vision_model)
video_file: file_types.File = genai.upload_file(str(video_path))
response: generation_types.GenerateContentResponse = self.model.generate_content([video_file, prompt])

The power of the Dark Side demands you migrate to the supported SDK. This is not a suggestion.

src/animawatch/browser.py (1)

158-160: ⚠️ Potential issue | 🟠 Major

The apprentice must learn proper parameter handling, or rebel scum shall exploit your code.

The f-string injection vulnerability is genuine and dangerous—unsanitised user input could corrupt your JavaScript execution. However, the proposed fix contains a critical flaw: Playwright's page.evaluate() requires passed arguments to be explicitly referenced in the JavaScript expression.

Correct the fix to:

await page.evaluate("y => window.scrollBy(0, y)", int(action.get("y", 500)))

The power of parameterised arguments reveals itself only when properly invoked. Coerce y to int and reference it in the JS expression to prevent both injection and runtime errors.

src/animawatch/server.py (1)

207-223: 🧹 Nitpick | 🔵 Trivial

The repetition of thy ctx-guard reveals rebel scum code, Apprentice.

Thou hast repeated the same if ctx is None: raise RuntimeError("Context is required") guard across five tools—a violation of DRY that clouds the code's power. FastMCP dependency-injects Context; these guards add defensive boilerplate where trust in the framework would suffice. Consider two paths to victory:

  1. Remove the guards entirely — FastMCP guarantees injection during execution; the optional parameter exists only for testing.
  2. Centralise via custom decorator — Wrap the guards in a single decorator to consolidate this rebel scum pattern across all tools.

Note: Thy @mcp.resource handlers at lines 166 and 176 use non-optional ctx without guards, proving the framework supports such strictness. Align thy tools with this approach.

This refactoring reveals the true power of the Dark Side: clean, maintainable code.

Affects: Lines 207–223, 264–276, 305–315, 337–350, 377–387.

🤖 Fix all issues with AI agents
In `@src/animawatch/browser.py`:
- Around line 81-85: Replace the untyped action payloads with a Pydantic
discriminated model: create a BaseModel (e.g., ActionBase) with a Literal field
"action" that can be "click" | "type" | "scroll" | "wait" | "hover" and then
define five subclasses (ClickAction, TypeAction, ScrollAction, WaitAction,
HoverAction) for the specific payload fields; change the signature of
record_interaction to accept actions: list[ActionBase] (or use
parse_obj_as(list[ActionBase], actions) inside the function) and update
_perform_action to accept a typed ActionBase and use its typed attributes
instead of dict lookups; add necessary imports from pydantic (BaseModel, Field,
validator, Literal, parse_obj_as) and ensure any callers pass validated models
or raw dicts that are converted via parse_obj_as before use.

In `@src/animawatch/vision.py`:
- Around line 45-48: The polling loop that waits for video processing (while
video_file.state.name == "PROCESSING") can hang indefinitely; add a hard timeout
or max poll count to fail fast: track elapsed time or iterations while calling
genai.get_file(video_file.name) and break/raise a specific exception (or set
state to FAILED) once the timeout or max retries is exceeded, and ensure any
callers handle that error; update the loop around video_file and genai.get_file
to include the timeout logic and a clear error path.

Comment thread src/animawatch/browser.py
Comment thread src/animawatch/vision.py
@coderabbitai

coderabbitai Bot commented Feb 2, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check for tempfile.mkstemp usage (security fix)
echo "=== 1. Checking for tempfile.mkstemp usage ==="
rg -n "mkstemp|mktemp" src/

echo -e "\n=== 2. Checking for os import ==="
rg -n "^import os$" src/

echo -e "\n=== 3. Checking for settings.video_size usage ==="
rg -n "settings\.video_size" src/

echo -e "\n=== 4. Checking for asyncio.sleep usage ==="
rg -n "asyncio\.sleep|time\.sleep" src/

echo -e "\n=== 5. Checking for contextlib.suppress usage ==="
rg -n "contextlib\.suppress" src/

echo -e "\n=== 6. Checking for noqa comments ==="
rg -n "noqa" src/

echo -e "\n=== 7. Checking for ViewportSize TypedDict ==="
rg -n "ViewportSize" src/

Length of output: 1425


@rishitank, excellent work, Apprentice. Your implementation is impressive. Most impressive.

I have verified all seven changes:

  1. ✅ Security fortified - tempfile.mkstemp deployed at line 134
  2. ✅ File descriptor handling - import os present
  3. ✅ Code discipline - settings.video_size utilized throughout
  4. ✅ Non-blocking operations - asyncio.sleep replaces all blocking calls
  5. ✅ Exception precision - contextlib.suppress(OSError) narrowed across all modules
  6. ✅ Clean imports - No unnecessary noqa comments remain
  7. ✅ Type safety - ViewportSize TypedDict properly defined

The power of the Dark Side flows through this codebase. Your discipline and attention to detail serve you well.


If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Actions performed

Comments resolved and changes approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant