feat(browser): add controller-level action blocklist#638
feat(browser): add controller-level action blocklist#638djcrafts wants to merge 5 commits intonottelabs:mainfrom
Conversation
…disallow specific action types and keyword-based blocking\n- Enforce policy in BrowserController.execute before running actions\n- Wire optional "blocklist" into NotteSession\n- Unit tests for type and keyword blocking
|
Warning Rate limit exceeded@djcrafts has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 55 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (3)
WalkthroughThis change adds an ActionBlocklist pydantic model with fields Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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: 0
🧹 Nitpick comments (3)
packages/notte-browser/src/notte_browser/session.py (1)
80-81: Consolidate imports from the same module.The two imports from
notte_browser.controllercan be combined into a single statement for cleaner code organization.🔎 Proposed fix
-from notte_browser.controller import BrowserController -from notte_browser.controller import ActionBlocklist +from notte_browser.controller import ActionBlocklist, BrowserControllertests/browser/test_blocklist.py (1)
29-59: Good test coverage for core scenarios.The tests effectively validate type-based blocking, keyword-based blocking with case-insensitivity, and non-matching cases. Consider adding edge case tests for completeness in a follow-up:
- Empty blocklist (should not block anything)
- Action with empty/None ID for keyword check (should not block)
- Multiple keywords where only one matches
packages/notte-browser/src/notte_browser/controller.py (1)
88-103: Consider logging swallowed exceptions for debuggability.The bare
except Exception: passblocks silently swallow all errors. While this defensive approach prevents blocking check failures from breaking execution, it makes debugging harder when text aggregation unexpectedly fails.🔎 Proposed improvement
+from notte_core.common.logging import logger + # Aggregate potential text sources for matching texts: list[str] = [] try: texts.append(node.inner_text()) -except Exception: - pass +except Exception as e: + logger.trace(f"Failed to get inner_text for node {action.id}: {e}") try: texts.append(node.text) -except Exception: - pass +except Exception as e: + logger.trace(f"Failed to get text for node {action.id}: {e}") try: if node.attributes is not None: for key in ("title", "aria_label", "name", "placeholder"): val = getattr(node.attributes, key, None) if isinstance(val, str): texts.append(val) -except Exception: - pass +except Exception as e: + logger.trace(f"Failed to get attributes for node {action.id}: {e}")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
packages/notte-browser/src/notte_browser/controller.py(3 hunks)packages/notte-browser/src/notte_browser/session.py(2 hunks)tests/browser/test_blocklist.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/notte-browser/src/notte_browser/controller.py (3)
packages/notte-core/src/notte_core/actions/actions.py (2)
BaseAction(59-137)InteractionAction(866-952)packages/notte-core/src/notte_core/browser/snapshot.py (1)
BrowserSnapshot(46-104)packages/notte-core/src/notte_core/errors/actions.py (1)
ActionExecutionError(10-21)
tests/browser/test_blocklist.py (5)
packages/notte-browser/src/notte_browser/controller.py (2)
ActionBlocklist(63-111)is_blocked(110-111)packages/notte-core/src/notte_core/browser/snapshot.py (2)
BrowserSnapshot(46-104)SnapshotMetadata(38-43)packages/notte-core/src/notte_core/browser/dom_tree.py (3)
DomNode(372-768)DomAttributes(150-350)ComputedDomAttributes(354-368)packages/notte-core/src/notte_core/browser/node_type.py (2)
NodeType(6-10)NodeRole(146-397)packages/notte-core/src/notte_core/actions/actions.py (3)
ClickAction(955-972)GotoAction(255-286)InteractionAction(866-952)
🪛 Ruff (0.14.8)
packages/notte-browser/src/notte_browser/controller.py
90-91: try-except-pass detected, consider logging the exception
(S110)
90-90: Do not catch blind exception: Exception
(BLE001)
94-95: try-except-pass detected, consider logging the exception
(S110)
94-94: Do not catch blind exception: Exception
(BLE001)
102-103: try-except-pass detected, consider logging the exception
(S110)
102-102: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (5)
packages/notte-browser/src/notte_browser/session.py (1)
118-126: LGTM!The optional
blocklistparameter is correctly added and forwarded toBrowserController. This maintains backward compatibility since the default isNone.tests/browser/test_blocklist.py (1)
9-26: LGTM!The helper function creates a minimal but sufficient
BrowserSnapshotfor testing blocklist behavior. The construction correctly sets up an interactive button node with all necessary attributes.packages/notte-browser/src/notte_browser/controller.py (3)
120-124: LGTM!The blocklist parameter is correctly added to the constructor and stored as an instance attribute. The optional nature maintains backward compatibility.
430-434: LGTM!The blocklist enforcement is correctly placed before action dispatch, preventing any side effects from blocked actions. The
ActionExecutionErroris appropriate here as it's already handled by the session layer (as seen insession.pylines 578-593).
73-74: LGTM!The type checking logic is clean and efficient. The
is_blockedmethod correctly short-circuits, checking the faster type-based blocking first before the more expensive keyword-based check.Also applies to: 110-111
…nnecessary None check and redundant isinstance in controller\n- Clean imports in test and organize imports via ruff\n- Run pre-commit locally; pushing formatting changes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/notte-browser/src/notte_browser/controller.py (2)
76-110: Consider logging exceptions for better debuggability.The keyword blocking logic is sound, but the bare
except Exception: passblocks (lines 89-104) silently swallow all errors, which could hide bugs during development.Note: The substring matching (
kw.lower() in text) will match partial words (e.g., "delete" matches "undeleted"). This appears intentional for broader coverage but could lead to false positives in edge cases.💡 Optional improvement: Add debug logging
try: texts.append(node.inner_text()) except Exception: + if config.verbose: + logger.debug(f"Failed to get inner_text for node {action.id}") pass try: texts.append(node.text) except Exception: + if config.verbose: + logger.debug(f"Failed to get text for node {action.id}") pass try: if node.attributes is not None: for key in ("title", "aria_label", "name", "placeholder"): val = getattr(node.attributes, key, None) if isinstance(val, str): texts.append(val) except Exception: + if config.verbose: + logger.debug(f"Failed to get attributes for node {action.id}") passBased on learnings, the static analysis warnings are noted but the defensive approach is reasonable for production robustness.
432-440: Enforcement logic correctly placed and implemented.The blocklist check is properly positioned before action execution, and
ActionExecutionErroris the appropriate error type for this failure mode.Minor enhancement suggestion: For
InteractionActionwith an emptyid, the error message usesaction_id="". Consider checking for empty strings to fall back toaction.typefor clearer diagnostics.💡 Optional improvement: Better action_id in error messages
if self.blocklist is not None and self.blocklist.is_blocked(action, prev_snapshot): # Raise an explicit error handled by session + action_id = getattr(action, "id", action.type) + if not action_id: + action_id = action.type raise ActionExecutionError( - action_id=getattr(action, "id", action.type), + action_id=action_id, url=window.page.url, reason="Unauthorized action (blocked by policy)", )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/notte-browser/src/notte_browser/controller.py(3 hunks)packages/notte-browser/src/notte_browser/session.py(2 hunks)tests/browser/test_blocklist.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/browser/test_blocklist.py
🧰 Additional context used
🧬 Code graph analysis (2)
packages/notte-browser/src/notte_browser/session.py (1)
packages/notte-browser/src/notte_browser/controller.py (2)
ActionBlocklist(63-113)BrowserController(117-471)
packages/notte-browser/src/notte_browser/controller.py (3)
packages/notte-core/src/notte_core/actions/actions.py (2)
BaseAction(59-137)InteractionAction(866-952)packages/notte-core/src/notte_core/browser/snapshot.py (1)
BrowserSnapshot(46-104)packages/notte-core/src/notte_core/browser/dom_tree.py (2)
find(445-454)inner_text(404-428)
🪛 Ruff (0.14.8)
packages/notte-browser/src/notte_browser/controller.py
91-92: try-except-pass detected, consider logging the exception
(S110)
91-91: Do not catch blind exception: Exception
(BLE001)
95-96: try-except-pass detected, consider logging the exception
(S110)
95-95: Do not catch blind exception: Exception
(BLE001)
103-104: try-except-pass detected, consider logging the exception
(S110)
103-103: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (7)
packages/notte-browser/src/notte_browser/session.py (3)
80-80: LGTM: Import added correctly.The
ActionBlocklistimport is properly used for the type hint on line 117.
117-117: LGTM: Backward-compatible parameter addition.The optional
blocklistparameter maintains backward compatibility while extending the API to support action blocking.
125-127: LGTM: Blocklist correctly forwarded to controller.The
blocklistparameter is properly passed toBrowserController, completing the integration from session to controller level.packages/notte-browser/src/notte_browser/controller.py (4)
44-44: LGTM: Pydantic imports added for new model.The
BaseModelandFieldimports are correctly added to support theActionBlocklistpydantic model.
73-74: LGTM: Type blocking logic is correct.The set membership check is efficient (O(1)) and straightforward. Note that
disallow_typesdoesn't validate against known action types, which provides flexibility for blocking any action type string.
112-113: LGTM: Combined blocking logic is correct.The
is_blockedmethod properly combines type and keyword blocking with OR logic, ensuring either condition triggers a block.
122-126: LGTM: Backward-compatible constructor extension.The optional
blocklistparameter maintains backward compatibility while enabling the new blocking feature.
… ImagingCore to Iterable of RGB tuples via object cast\n- Use collections.abc.Iterable to avoid deprecation warning\n- Remove unnecessary type ignore in webvoyager convert script\n- Run pre-commit: ruff sorted imports
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/notte-core/src/notte_core/browser/highlighter.py (1)
145-146: Verify that Pillow 11.1.0 type stubs or documentation support direct typing of getdata() for RGB images, or document the workaround.Pillow 11.1.0 includes type annotations and a py.typed marker, but Image.getdata() is typed to return core.ImagingCore rather than a specifically-typed iterable. The double cast through object is a workaround for converting this internal C extension type to
list[tuple[int, int, int]]for RGB images. Since type stubs don't provide mode-aware return types for getdata(), this pattern remains necessary. Consider adding a code comment explaining that the cast sequence is required due to Pillow's internal type limitations, or verify if a cleaner approach exists in your type checking configuration.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
packages/notte-core/src/notte_core/browser/highlighter.py(2 hunks)packages/notte-eval/src/notte_eval/data/webvoyager/convert.py(1 hunks)
🔇 Additional comments (2)
packages/notte-core/src/notte_core/browser/highlighter.py (1)
2-2: LGTM: Typing imports added for explicit type annotations.The addition of
Iterablefromcollections.abcandcastfromtypingsupports the explicit type handling in the pixel data extraction logic below.Also applies to: 5-5
packages/notte-eval/src/notte_eval/data/webvoyager/convert.py (1)
23-23: Removal of# type: ignoreis appropriate.The DataFrame.to_json typing issue previously reported was specific to binary buffers, not to string file paths. Since the code passes a string path with
orient="records"andlines=True—a fully supported combination where the method accepts string path objects and writes out line-delimited JSON format—the typing is well-defined. The project uses pandas-stubs with versions aligned to recent pandas releases, ensuring proper type support for this standard usage pattern.
…rements.txt (not tracked upstream)\n- Restore uv.lock from upstream main\n- Silence basedpyright warnings with targeted ignores and stable typing\n- Keep blocklist feature files unchanged
…n\n- Add collection hook to skip tests under tests/integration by default\n- Keep unit tests and browser blocklist tests unaffected\n- Address CodeRabbit nitpick with explanatory comment
About the integration test skipI added a pytest hook that skips integration tests by default unless Why? Integration tests require:
On forks without credentials, these tests fail with confusing errors. This change:
Basically: faster CI feedback for external contributors, no noise from expected failures. |
1 participant
feat(browser): add controller-level action blocklist
Fixes #231
Summary:
ActionBlocklistto enforce action restrictions at the browser controller level.disallow_types) and by keywords matched against element text/metadata (keywords).BrowserController.execute(), raising a handledActionExecutionErrorif blocked.blocklistintoNotteSession.Motivation:
Implements request to add a blocklist for the browser controller so the agent cannot execute unauthorized actions or actions including specified keywords.
Changes:
ActionBlocklistmodel; enforcement logic inexecute().blocklistparameter wired to controller.Testing:
gotoaction fails with a clear error.Backward compatibility:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.