Conversation
WalkthroughThe pull request introduces persona and vault integration across multiple packages. The notte-agent package adds duplicate prevention logic to avoid creating multiple PersonaTool instances for the same persona. The notte-browser session class now accepts vault and persona as optional constructor parameters, includes methods to attach personas and manage vaults, and applies vault-based credential replacement during action execution. New convenience methods for reading emails and SMS messages are added to the session class. The notte-core credentials module exports eight credential type constants (EMAIL, USERNAME, PASSWORD, MFA, CARD_NUMBER, CARD_HOLDER_NAME, CARD_EXPIRATION, CARD_CVV), which are then re-exported at the top-level notte module for public API access. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _has_persona_tool(self, persona: BasePersona) -> bool: | ||
| for tool in self.tools: | ||
| if not isinstance(tool, PersonaTool): | ||
| continue | ||
| try: | ||
| if tool.persona.info.persona_id == persona.info.persona_id: | ||
| return True | ||
| except Exception: | ||
| if tool.persona is persona: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Bare except catches all exceptions on line 154, potentially hiding real errors like AttributeError when tool.persona doesn't have info attribute
| def _has_persona_tool(self, persona: BasePersona) -> bool: | |
| for tool in self.tools: | |
| if not isinstance(tool, PersonaTool): | |
| continue | |
| try: | |
| if tool.persona.info.persona_id == persona.info.persona_id: | |
| return True | |
| except Exception: | |
| if tool.persona is persona: | |
| return True | |
| return False | |
| def _has_persona_tool(self, persona: BasePersona) -> bool: | |
| for tool in self.tools: | |
| if not isinstance(tool, PersonaTool): | |
| continue | |
| if tool.persona.info.persona_id == persona.info.persona_id: | |
| return True | |
| return False |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/notte-browser/src/notte_browser/session.py
Line: 147:157
Comment:
Bare except catches all exceptions on line 154, potentially hiding real errors like `AttributeError` when `tool.persona` doesn't have `info` attribute
```suggestion
def _has_persona_tool(self, persona: BasePersona) -> bool:
for tool in self.tools:
if not isinstance(tool, PersonaTool):
continue
if tool.persona.info.persona_id == persona.info.persona_id:
return True
return False
```
How can I resolve this? If you propose a fix, please make it concise.| if not any( | ||
| isinstance(tool, PersonaTool) | ||
| and getattr(tool.persona.info, "persona_id", None) == getattr(persona.info, "persona_id", None) | ||
| for tool in self.tools | ||
| ): | ||
| self.tools.append(PersonaTool(persona)) |
There was a problem hiding this comment.
Uses getattr with fallback to None for persona_id, but this masks real AttributeError if info exists but has the wrong structure
| if not any( | |
| isinstance(tool, PersonaTool) | |
| and getattr(tool.persona.info, "persona_id", None) == getattr(persona.info, "persona_id", None) | |
| for tool in self.tools | |
| ): | |
| self.tools.append(PersonaTool(persona)) | |
| if not any( | |
| isinstance(tool, PersonaTool) and tool.persona.info.persona_id == persona.info.persona_id | |
| for tool in self.tools | |
| ): |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/notte-agent/src/notte_agent/main.py
Line: 56:61
Comment:
Uses `getattr` with fallback to `None` for `persona_id`, but this masks real `AttributeError` if `info` exists but has the wrong structure
```suggestion
if not any(
isinstance(tool, PersonaTool) and tool.persona.info.persona_id == persona.info.persona_id
for tool in self.tools
):
```
How can I resolve this? If you propose a fix, please make it concise.| self.persona: BasePersona | None = persona | ||
| if persona is not None: | ||
| self.attach_persona(persona) |
There was a problem hiding this comment.
Check that vault from persona is attached - NotteSession doesn't set self.vault = persona.vault when persona has a vault (unlike Agent which does)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/notte-browser/src/notte_browser/session.py
Line: 136:138
Comment:
Check that vault from persona is attached - `NotteSession` doesn't set `self.vault = persona.vault` when persona has a vault (unlike `Agent` which does)
How can I resolve this? If you propose a fix, please make it concise.![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notte/__init__.py (1)
7-40:⚠️ Potential issue | 🟡 MinorRemove undocumented credential constant re-exports or document their purpose.
The credential constants (EMAIL, USERNAME, PASSWORD, MFA, CARD_NUMBER, CARD_HOLDER_NAME, CARD_EXPIRATION, CARD_CVV) are now exposed at the package root via
__all__, but this re-export is neither documented in the README nor used in any code examples. These are test/placeholder values, not production-ready. Either remove these from__all__or add clear documentation explaining why users should import them directly fromnotte.
🤖 Fix all issues with AI agents
In `@packages/notte-agent/src/notte_agent/main.py`:
- Around line 56-61: The current de-dup check in the Agent constructor accesses
persona.info and can raise (e.g., _get_info failure), causing Agent creation to
hard-fail; wrap persona.info access in a safe check (try/except) or use
identity-based fallback so failures to fetch info do not raise: in the block
that builds PersonaTool entries (referencing PersonaTool, self.tools, and
persona.info), attempt to read persona.info inside a try and only compare
persona_id when available, otherwise fall back to comparing persona object
identity (or delegate to NotteSession._has_persona_tool if available) before
appending a new PersonaTool(persona).
In `@packages/notte-browser/src/notte_browser/session.py`:
- Around line 135-138: The session currently assigns self.vault from the
explicit vault argument and then attaches persona, so when callers pass persona
but omit vault credential replacement never happens; update the initializer so
that if vault is None and a persona is provided you set self.vault =
persona.vault (or persona.get_vault() if accessor exists) before calling
self.attach_persona(persona), ensuring BasePersona's vault is used as the
session vault and that attach_persona continues to operate against self.vault.
In `@packages/notte-core/src/notte_core/credentials/__init__.py`:
- Around line 1-9: The constants EMAIL, USERNAME, PASSWORD, MFA, CARD_NUMBER,
CARD_HOLDER_NAME, CARD_EXPIRATION and CARD_CVV in notte_core.credentials contain
real-looking PII/secrets; replace their string values with non-sensitive
placeholders (e.g., "EMAIL_PLACEHOLDER", "USERNAME_PLACEHOLDER",
"PASSWORD_PLACEHOLDER", "MFA_PLACEHOLDER", "CARD_NUMBER_PLACEHOLDER",
"CARD_HOLDER_NAME_PLACEHOLDER", "CARD_EXPIRATION_PLACEHOLDER",
"CARD_CVV_PLACEHOLDER") and remove any misleading real-format data; keep them as
clear identifiers so they represent credential types only and do not contain
real or realistic secrets.
🧹 Nitpick comments (2)
packages/notte-core/src/notte_core/credentials/__init__.py (1)
11-20: Sort__all__to satisfy Ruff (RUF022).Optional, but this warning will keep surfacing in lint runs.
♻️ Suggested ordering
__all__ = [ - "EMAIL", - "USERNAME", - "PASSWORD", - "MFA", "CARD_NUMBER", "CARD_HOLDER_NAME", "CARD_EXPIRATION", "CARD_CVV", + "EMAIL", + "MFA", + "PASSWORD", + "USERNAME", ]packages/notte-browser/src/notte_browser/session.py (1)
474-491: Avoid re-resolving an already-resolved action just to locate elements.
_action_with_vault()callsself.locate(action), which re-runsNodeResolutionPipe.forward. Sinceresolved_actionalready exists, prefer using its selectors directly to avoid extra work and potential drift.♻️ Suggested refactor
- locator = await self.locate(action) + locator = None + if isinstance(action, InteractionAction) and action.selectors is not None: + locator = await locate_element(self.window.page, action.selectors) + else: + locator = await self.locate(action)
| if not any( | ||
| isinstance(tool, PersonaTool) | ||
| and getattr(tool.persona.info, "persona_id", None) == getattr(persona.info, "persona_id", None) | ||
| for tool in self.tools | ||
| ): | ||
| self.tools.append(PersonaTool(persona)) |
There was a problem hiding this comment.
Avoid hard failure when persona.info lookup fails.
Line 56 now calls persona.info during de-dupe. If _get_info() raises (network, auth, etc.), Agent creation will fail — a new regression. Consider a safe fallback (object identity) like NotteSession._has_persona_tool.
🐛 Suggested hardening
- if not any(
- isinstance(tool, PersonaTool)
- and getattr(tool.persona.info, "persona_id", None) == getattr(persona.info, "persona_id", None)
- for tool in self.tools
- ):
+ try:
+ persona_id = getattr(persona.info, "persona_id", None)
+ def _match(tool: BaseTool) -> bool:
+ return (
+ isinstance(tool, PersonaTool)
+ and getattr(tool.persona.info, "persona_id", None) == persona_id
+ )
+ except Exception:
+ def _match(tool: BaseTool) -> bool:
+ return isinstance(tool, PersonaTool) and tool.persona is persona
+
+ if not any(_match(tool) for tool in self.tools):
self.tools.append(PersonaTool(persona))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not any( | |
| isinstance(tool, PersonaTool) | |
| and getattr(tool.persona.info, "persona_id", None) == getattr(persona.info, "persona_id", None) | |
| for tool in self.tools | |
| ): | |
| self.tools.append(PersonaTool(persona)) | |
| try: | |
| persona_id = getattr(persona.info, "persona_id", None) | |
| def _match(tool: BaseTool) -> bool: | |
| return ( | |
| isinstance(tool, PersonaTool) | |
| and getattr(tool.persona.info, "persona_id", None) == persona_id | |
| ) | |
| except Exception: | |
| def _match(tool: BaseTool) -> bool: | |
| return isinstance(tool, PersonaTool) and tool.persona is persona | |
| if not any(_match(tool) for tool in self.tools): | |
| self.tools.append(PersonaTool(persona)) |
🤖 Prompt for AI Agents
In `@packages/notte-agent/src/notte_agent/main.py` around lines 56 - 61, The
current de-dup check in the Agent constructor accesses persona.info and can
raise (e.g., _get_info failure), causing Agent creation to hard-fail; wrap
persona.info access in a safe check (try/except) or use identity-based fallback
so failures to fetch info do not raise: in the block that builds PersonaTool
entries (referencing PersonaTool, self.tools, and persona.info), attempt to read
persona.info inside a try and only compare persona_id when available, otherwise
fall back to comparing persona object identity (or delegate to
NotteSession._has_persona_tool if available) before appending a new
PersonaTool(persona).
| self.vault: BaseVault | None = vault | ||
| self.persona: BasePersona | None = persona | ||
| if persona is not None: | ||
| self.attach_persona(persona) |
There was a problem hiding this comment.
Persona-provided vault isn’t wired into the session.
If callers pass persona but omit vault, credential replacement is silently disabled. Consider mirroring Agent behavior by adopting persona.vault when available.
🐛 Suggested fix
self.vault: BaseVault | None = vault
self.persona: BasePersona | None = persona
if persona is not None:
+ if self.vault is None and persona.has_vault:
+ self.vault = persona.vault
self.attach_persona(persona)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.vault: BaseVault | None = vault | |
| self.persona: BasePersona | None = persona | |
| if persona is not None: | |
| self.attach_persona(persona) | |
| self.vault: BaseVault | None = vault | |
| self.persona: BasePersona | None = persona | |
| if persona is not None: | |
| if self.vault is None and persona.has_vault: | |
| self.vault = persona.vault | |
| self.attach_persona(persona) |
🤖 Prompt for AI Agents
In `@packages/notte-browser/src/notte_browser/session.py` around lines 135 - 138,
The session currently assigns self.vault from the explicit vault argument and
then attaches persona, so when callers pass persona but omit vault credential
replacement never happens; update the initializer so that if vault is None and a
persona is provided you set self.vault = persona.vault (or persona.get_vault()
if accessor exists) before calling self.attach_persona(persona), ensuring
BasePersona's vault is used as the session vault and that attach_persona
continues to operate against self.vault.
1 participant
Greptile Overview
Greptile Summary
This PR integrates vault (credentials) and persona support into the session layer, allowing credentials to be automatically replaced in form-filling actions.
Major changes:
NotteSessionnow acceptsvaultandpersonaparameters, with methods to read emails/SMS and attach personas_action_with_vaultmethodPersonaToolprevention logic added in bothAgentandNotteSessionIssues found:
exceptclause in_has_persona_toolmay hide unexpected errorsAgentandNotteSession: Agent auto-attaches persona's vault, Session doesn'tConfidence Score: 3/5
packages/notte-browser/src/notte_browser/session.py- verify error handling and vault attachment behaviorImportant Files Changed
Summary by CodeRabbit
New Features
read_emails()andread_sms()methods for accessing user communication dataBug Fixes