feat(credentials): add BitwardenVault and AacVault integrations#816
Open
giordano-lucas wants to merge 8 commits into
Open
feat(credentials): add BitwardenVault and AacVault integrations#816giordano-lucas wants to merge 8 commits into
giordano-lucas wants to merge 8 commits into
Conversation
Add two new BaseVault implementations that allow notte agents to use external credential providers instead of the built-in cloud vault: - BitwardenVault: wraps the `bws` CLI to fetch credentials from Bitwarden Secrets Manager projects. Bulk-loads secrets on start and matches by root domain. - AacVault: wraps the `aac` CLI (Agent Access Protocol) to fetch credentials on-demand through an E2E encrypted Noise tunnel. Works with any aac-compatible provider (Bitwarden Password Manager, etc.). Overrides get_credentials_async to handle live TOTP codes. Both are drop-in replacements for NotteVault in local agent mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds two new vault implementations in notte-core: AacVault (uses the Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| asyncio.run(vault.get_credentials_async("https://github.com")) | ||
| past = vault.past_credentials() | ||
| assert len(past) == 1 | ||
| assert "https://github.com" in past |
| assert len(cred_list) == 2 | ||
| urls = {c.url for c in cred_list} | ||
| assert "https://github.com/login" in urls | ||
| assert "https://app.notte.cc" in urls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/notte-core/src/notte_core/credentials/aac.py`:
- Around line 131-145: The code in _get_credentials_impl assumes the returned
cred from _request_credential(domain) is a mapping and calls cred.get(...),
which can raise if cred is not a dict; validate the payload type first (e.g.,
check isinstance(cred, dict) or typing.Mapping) and return None (or handle/log
appropriately) when it's not the expected object before accessing fields like
"password"; update the logic around the cred variable in _get_credentials_impl
to perform this type check and then safely extract password and other fields.
In `@packages/notte-core/src/notte_core/credentials/bitwarden.py`:
- Line 94: The code treats an empty list in self._secrets_cache as a cache miss
by using "self._secrets_cache or self._fetch_secrets()", causing repeated
refetches; change checks to explicitly test for None (e.g., if
self._secrets_cache is None then call self._fetch_secrets() and assign the
result) wherever this pattern appears (references: self._secrets_cache and
_fetch_secrets in the methods around the occurrences that currently use the "or"
idiom, including the spots flagged at lines near 94, 115, and 130).
- Around line 54-60: The _run_bws helper currently calls subprocess.run without
a timeout which can hang; update _run_bws to pass a sensible timeout (e.g., a
few seconds) to subprocess.run and catch subprocess.TimeoutExpired around that
call (referencing _run_bws, subprocess.run, self._bws_path, self._access_token)
so the process is terminated and a clear RuntimeError is raised describing the
timeout (include any partial output or stderr if available). Ensure the env and
other args remain unchanged and that non-timeout failures still raise the
existing RuntimeError on non-zero return codes.
In `@tests/test_aac_vault.py`:
- Around line 169-175: The test test_reads_token_from_env mutates os.environ
directly; replace that with pytest's monkeypatch to avoid leaking AAC_TOKEN
state: use monkeypatch.setenv("AAC_TOKEN", "ENV-TOK-123") before instantiating
AacVault(aac_path=fake_aac) and rely on monkeypatch to restore/clear the
variable (or use monkeypatch.delenv in case you need explicit removal), so the
test asserts vault.token == "ENV-TOK-123" without touching os.environ directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c71f0f80-4a2c-4d02-9df5-a3a027091dc0
📒 Files selected for processing (6)
examples/auth-aac-agent/agent.pyexamples/auth-bitwarden-agent/agent.pypackages/notte-core/src/notte_core/credentials/aac.pypackages/notte-core/src/notte_core/credentials/bitwarden.pytests/test_aac_vault.pytests/test_bitwarden_vault.py
Comment on lines
+131
to
+145
| return data.get("credential") | ||
|
|
||
| @override | ||
| async def _get_credentials_impl(self, url: str) -> CredentialsDict | None: | ||
| domain = get_root_domain(url) | ||
| if not domain: | ||
| return None | ||
|
|
||
| cred = self._request_credential(domain) | ||
| if cred is None: | ||
| return None | ||
|
|
||
| password = cred.get("password") | ||
| if not password: | ||
| return None |
Contributor
There was a problem hiding this comment.
Validate credential payload type before accessing fields.
If credential is not an object, this path can raise at runtime when calling .get(...).
Suggested fix
- return data.get("credential")
+ credential = data.get("credential")
+ if not isinstance(credential, dict):
+ logger.warning(f"[AacVault] Invalid credential payload for {domain}")
+ return None
+ return credential🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/notte-core/src/notte_core/credentials/aac.py` around lines 131 -
145, The code in _get_credentials_impl assumes the returned cred from
_request_credential(domain) is a mapping and calls cred.get(...), which can
raise if cred is not a dict; validate the payload type first (e.g., check
isinstance(cred, dict) or typing.Mapping) and return None (or handle/log
appropriately) when it's not the expected object before accessing fields like
"password"; update the logic around the cred variable in _get_credentials_impl
to perform this type check and then safely extract password and other fields.
Comment on lines
+169
to
+175
| def test_reads_token_from_env(self, fake_aac: str) -> None: | ||
| os.environ["AAC_TOKEN"] = "ENV-TOK-123" | ||
| try: | ||
| vault = AacVault(aac_path=fake_aac) | ||
| assert vault.token == "ENV-TOK-123" | ||
| finally: | ||
| del os.environ["AAC_TOKEN"] |
Contributor
There was a problem hiding this comment.
Use monkeypatch for env var isolation in tests.
Direct os.environ mutation here can leak state when AAC_TOKEN already exists.
Suggested fix
- def test_reads_token_from_env(self, fake_aac: str) -> None:
- os.environ["AAC_TOKEN"] = "ENV-TOK-123"
- try:
- vault = AacVault(aac_path=fake_aac)
- assert vault.token == "ENV-TOK-123"
- finally:
- del os.environ["AAC_TOKEN"]
+ def test_reads_token_from_env(self, fake_aac: str, monkeypatch: pytest.MonkeyPatch) -> None:
+ monkeypatch.setenv("AAC_TOKEN", "ENV-TOK-123")
+ vault = AacVault(aac_path=fake_aac)
+ assert vault.token == "ENV-TOK-123"🧰 Tools
🪛 Ruff (0.15.12)
[error] 170-170: Possible hardcoded password assigned to: "AAC_TOKEN"
(S105)
[error] 173-173: Possible hardcoded password assigned to: "token"
(S105)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_aac_vault.py` around lines 169 - 175, The test
test_reads_token_from_env mutates os.environ directly; replace that with
pytest's monkeypatch to avoid leaking AAC_TOKEN state: use
monkeypatch.setenv("AAC_TOKEN", "ENV-TOK-123") before instantiating
AacVault(aac_path=fake_aac) and rely on monkeypatch to restore/clear the
variable (or use monkeypatch.delenv in case you need explicit removal), so the
test asserts vault.token == "ENV-TOK-123" without touching os.environ directly.
|
| Filename | Overview |
|---|---|
| packages/notte-core/src/notte_core/credentials/aac.py | New AacVault: async _get_credentials_impl wraps a blocking subprocess.run (previously flagged), but logic and session/pairing handling look correct |
| packages/notte-core/src/notte_core/credentials/bitwarden.py | New BitwardenVault: bulk-loads secrets on start(), cache uses correct is not None check, timeout added to subprocess.run, all looks correct |
| tests/test_aac_vault.py | Good unit test coverage; fake aac script uses echo with single-quoted JSON which breaks on passwords containing single quotes (bitwarden fixture was already fixed to use cat+tempfile) |
| tests/test_bitwarden_vault.py | Good coverage; _make_fake_bws leaks the data temp file — only the shell script is unlinked by the fixture, not the backing JSON file |
| tests/integration/test_aac_vault_integration.py | Properly gated integration test; skipped unless AAC_INTEGRATION_TEST=1 and aac CLI are present |
| tests/integration/test_bitwarden_vault_integration.py | Properly gated integration test; skipped unless BWS_ACCESS_TOKEN and bws CLI are present |
| examples/auth-aac-agent/agent.py | Clean example script demonstrating AacVault usage |
| examples/auth-bitwarden-agent/agent.py | Clean example script demonstrating BitwardenVault usage |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
tests/test_aac_vault.py:11-50
The `echo '...'` pattern in `_make_fake_aac` embeds the JSON directly into single-quoted shell strings. If any credential value contains a single quote (e.g., a password like `it's-mine`), the generated shell script becomes syntactically invalid and silently produces garbage output. The bitwarden fixture was fixed to use a temp file + `cat`, but this one was not.
```suggestion
def _make_fake_aac(credential: dict | None = None, fail: bool = False) -> str:
"""Create a fake aac script that simulates the aac CLI."""
cred = credential or {
"username": "octocat",
"password": "gh-secret-123", # pragma: allowlist secret
"totp": "654321",
"uri": "https://github.com",
"notes": None,
}
if fail:
pairing_json = '{"error":{"code":"general_error","message":"connection failed"},"success":false}'
success_json = pairing_json
else:
success_json = json.dumps({"credential": cred, "domain": "github.com", "success": True})
pairing_json = json.dumps({"success": True})
success_file = tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False)
success_file.write(success_json)
success_file.close()
pairing_file = tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False)
pairing_file.write(pairing_json)
pairing_file.close()
f = tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=False)
if fail:
f.write(
f"#!/bin/bash\n"
f"cat {pairing_file.name}\n"
f"exit 2\n"
)
else:
f.write(
f"#!/bin/bash\n"
f'if echo "$@" | grep -q "\\-\\-domain"; then\n'
f" cat {success_file.name}\n"
f" exit 0\n"
f"fi\n"
f"cat {pairing_file.name}\n"
f"exit 0\n"
)
f.close()
os.chmod(f.name, stat.S_IRWXU)
return f.name
```
### Issue 2 of 2
tests/test_bitwarden_vault.py:50-66
The `data_file` (JSON temp file) is created with `delete=False` but never unlinked. The fixture only cleans up the shell script (`path`). Each test invocation that uses this fixture leaks one temp `.json` file on disk.
```suggestion
def _make_fake_bws(secrets: list[dict]) -> tuple[str, str]:
"""Create a fake bws script that returns given secrets as JSON.
Returns (script_path, data_path) so callers can clean up both files.
"""
data_file = tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False)
data_file.write(json.dumps(secrets))
data_file.close()
f = tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=False)
f.write(f"#!/bin/bash\ncat {data_file.name}\n")
f.close()
os.chmod(f.name, stat.S_IRWXU)
return f.name, data_file.name
@pytest.fixture()
def fake_bws():
path, data_path = _make_fake_bws([GITHUB_SECRET, NOTTE_SECRET, INVALID_SECRET, MISSING_PASSWORD_SECRET])
yield path
os.unlink(path)
os.unlink(data_path)
```
Reviews (4): Last reviewed commit: "address greptile review feedback (greplo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![mendral-app[bot]](https://avatars.githubusercontent.com/in/1975249?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Add configurable timeout to BitwardenVault._run_bws (default 30s) - Fix test fixture single-quote fragility (use cat from temp file) - Add integration tests gated behind BWS_ACCESS_TOKEN and AAC_INTEGRATION_TEST Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/notte-core/src/notte_core/credentials/bitwarden.py (2)
96-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty cache is treated as cache miss, causing unnecessary refetches.
Using
self._secrets_cache or self._fetch_secrets()treats an empty list[]as falsy, forcing refetches even when the cache was successfully populated but empty.🛡️ Proposed fix
- secrets = self._secrets_cache or self._fetch_secrets() + secrets = self._secrets_cache if self._secrets_cache is not None else self._fetch_secrets()Also applies to lines 117 and 132.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/notte-core/src/notte_core/credentials/bitwarden.py` at line 96, The code treats an empty cached value as a miss because it does `self._secrets_cache or self._fetch_secrets()`, causing unnecessary refetches; change this to explicitly check for None (e.g., `self._secrets_cache if self._secrets_cache is not None else self._fetch_secrets()`) wherever that pattern appears (the uses of `self._secrets_cache` paired with `_fetch_secrets()`), so an intentionally empty cache (like `[]`) is respected while still triggering `_fetch_secrets()` only when the cache is truly unset.
94-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlocking subprocess call in async method will stall the event loop.
The
_fetch_secrets()method callssubprocess.run()synchronously, which will block the event loop for the duration of thebwsCLI call. This can degrade performance and responsiveness in async contexts.🔄 Proposed fix using asyncio.to_thread
`@override` async def _get_credentials_impl(self, url: str) -> CredentialsDict | None: - secrets = self._secrets_cache or self._fetch_secrets() + import asyncio + secrets = self._secrets_cache if self._secrets_cache is not None else await asyncio.to_thread(self._fetch_secrets) target_domain = get_root_domain(url)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/notte-core/src/notte_core/credentials/bitwarden.py` around lines 94 - 105, The method _get_credentials_impl calls the synchronous _fetch_secrets which internally uses subprocess.run and will block the event loop; change the call to run _fetch_secrets off the event loop (e.g., await asyncio.to_thread(self._fetch_secrets)) or convert _fetch_secrets into an async function that uses asyncio.create_subprocess_exec so the bws CLI is executed non-blockingly; update the call sites (including where _secrets_cache is assigned/used) to await the new async behavior and ensure the rest of _get_credentials_impl still handles a None/empty secrets result.
🧹 Nitpick comments (1)
packages/notte-core/src/notte_core/credentials/bitwarden.py (1)
57-57: 💤 Low valueConsider unpacking syntax for list construction.
The static analyzer suggests using
[self._bws_path, *list(args), "--output", "json"]instead of concatenation for slightly cleaner code.♻️ Suggested refactor
- cmd = [self._bws_path] + list(args) + ["--output", "json"] + cmd = [self._bws_path, *list(args), "--output", "json"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/notte-core/src/notte_core/credentials/bitwarden.py` at line 57, The list construction for cmd should use sequence unpacking instead of concatenation: replace the expression that builds cmd (currently using [self._bws_path] + list(args) + ["--output", "json"]) with a single list literal that unpacks args (e.g., [self._bws_path, *args, "--output", "json"]) to be clearer and avoid the extra list() allocation; update the code in the method that defines cmd within the Bitwarden credential helper where self._bws_path and args are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/notte-core/src/notte_core/credentials/bitwarden.py`:
- Around line 107-113: The _add_credentials async method calls the blocking
helper _run_bws directly (causing event-loop blocking); change the call to run
in a thread and await it (e.g., await asyncio.to_thread(self._run_bws, "secret",
"create", url, value, self._project_id)), keep the existing ValueError check for
_project_id, preserve setting self._secrets_cache = None after the awaited call,
and ensure imports/typing include asyncio if not already present so
_add_credentials remains non-blocking.
- Around line 130-146: The async method list_credentials_async calls the
potentially blocking helper _fetch_secrets() directly; change it to run in a
background thread using asyncio.to_thread and await its result (e.g., secrets =
self._secrets_cache or await asyncio.to_thread(self._fetch_secrets)) so the
event loop isn't blocked. Update any logic that expects _fetch_secrets to be
synchronous inside list_credentials_async (keep parsing via _parse_secret_value
and building Credential instances unchanged) and ensure you import asyncio if
not already present.
- Around line 115-128: delete_credentials_async is performing blocking work by
calling the synchronous helpers _run_bws(...) and _fetch_secrets() inside an
async method; change it to run those blocking calls on the default threadpool
(e.g., via asyncio.to_thread) so the event loop isn't blocked: call secrets =
self._secrets_cache or await asyncio.to_thread(self._fetch_secrets), and replace
the blocking call _run_bws("secret", "delete", secret["id"]) with await
asyncio.to_thread(self._run_bws, "secret", "delete", secret["id"]); keep
existing logic around _parse_secret_value, _secrets_cache invalidation and the
ValueError unchanged.
In `@tests/integration/test_aac_vault_integration.py`:
- Around line 31-35: The test creates an AacVault, calls vault.start(), uses
asyncio.run(vault.list_credentials_async()), asserts results, then calls
vault.stop(); wrap the start/use/stop sequence in a try/finally so vault.stop()
is executed even if list_credentials_async or the assertion raises. Locate the
AacVault instantiation and calls to start, list_credentials_async (via
asyncio.run) and stop, call start before the try, perform the async call and
assertion inside the try block, and call vault.stop() in the finally block to
guarantee cleanup.
---
Duplicate comments:
In `@packages/notte-core/src/notte_core/credentials/bitwarden.py`:
- Line 96: The code treats an empty cached value as a miss because it does
`self._secrets_cache or self._fetch_secrets()`, causing unnecessary refetches;
change this to explicitly check for None (e.g., `self._secrets_cache if
self._secrets_cache is not None else self._fetch_secrets()`) wherever that
pattern appears (the uses of `self._secrets_cache` paired with
`_fetch_secrets()`), so an intentionally empty cache (like `[]`) is respected
while still triggering `_fetch_secrets()` only when the cache is truly unset.
- Around line 94-105: The method _get_credentials_impl calls the synchronous
_fetch_secrets which internally uses subprocess.run and will block the event
loop; change the call to run _fetch_secrets off the event loop (e.g., await
asyncio.to_thread(self._fetch_secrets)) or convert _fetch_secrets into an async
function that uses asyncio.create_subprocess_exec so the bws CLI is executed
non-blockingly; update the call sites (including where _secrets_cache is
assigned/used) to await the new async behavior and ensure the rest of
_get_credentials_impl still handles a None/empty secrets result.
---
Nitpick comments:
In `@packages/notte-core/src/notte_core/credentials/bitwarden.py`:
- Line 57: The list construction for cmd should use sequence unpacking instead
of concatenation: replace the expression that builds cmd (currently using
[self._bws_path] + list(args) + ["--output", "json"]) with a single list literal
that unpacks args (e.g., [self._bws_path, *args, "--output", "json"]) to be
clearer and avoid the extra list() allocation; update the code in the method
that defines cmd within the Bitwarden credential helper where self._bws_path and
args are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8991cd48-19fe-4807-b47a-b72526c9c7b4
📒 Files selected for processing (4)
packages/notte-core/src/notte_core/credentials/bitwarden.pytests/integration/test_aac_vault_integration.pytests/integration/test_bitwarden_vault_integration.pytests/test_bitwarden_vault.py
Comment on lines
+31
to
+35
| vault = AacVault(token=None) | ||
| vault.start() | ||
| creds = asyncio.run(vault.list_credentials_async()) | ||
| assert creds == [] | ||
| vault.stop() |
Contributor
There was a problem hiding this comment.
Ensure vault.stop() always runs via try/finally.
If the call or assertion fails, cleanup is skipped and the cached session/process state can leak across integration runs. Wrap start/use/stop with try/finally.
Suggested change
def test_aac_connect_with_cached_session() -> None:
"""Test credential fetch using a cached aac session (no token needed)."""
vault = AacVault(token=None)
- vault.start()
- creds = asyncio.run(vault.list_credentials_async())
- assert creds == []
- vault.stop()
+ vault.start()
+ try:
+ creds = asyncio.run(vault.list_credentials_async())
+ assert creds == []
+ finally:
+ vault.stop()📝 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.
Suggested change
| vault = AacVault(token=None) | |
| vault.start() | |
| creds = asyncio.run(vault.list_credentials_async()) | |
| assert creds == [] | |
| vault.stop() | |
| def test_aac_connect_with_cached_session() -> None: | |
| """Test credential fetch using a cached aac session (no token needed).""" | |
| vault = AacVault(token=None) | |
| vault.start() | |
| try: | |
| creds = asyncio.run(vault.list_credentials_async()) | |
| assert creds == [] | |
| finally: | |
| vault.stop() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/test_aac_vault_integration.py` around lines 31 - 35, The
test creates an AacVault, calls vault.start(), uses
asyncio.run(vault.list_credentials_async()), asserts results, then calls
vault.stop(); wrap the start/use/stop sequence in a try/finally so vault.stop()
is executed even if list_credentials_async or the assertion raises. Locate the
AacVault instantiation and calls to start, list_credentials_async (via
asyncio.run) and stop, call start before the try, perform the async call and
assertion inside the try block, and call vault.stop() in the finally block to
guarantee cleanup.
Member
Author
|
@greptile new review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Validate success field in aac pairing response, not just exit code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
@greptile new review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Fix empty list falsy bug: use `is not None` check for _secrets_cache so an empty project doesn't bypass the cache on every call Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
@greptile new review |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/notte-core/src/notte_core/credentials/bitwarden.py (1)
95-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid blocking the event loop in async vault methods.
Line 96, Line 112, Line 117, Line 125, and Line 132 still run synchronous CLI work inside
asyncmethods. This can stall unrelated coroutines during slowbwscalls. Please offload_fetch_secrets/_run_bwsviaasyncio.to_thread(...)in these async paths.Also applies to: 108-113, 116-117, 125-126, 131-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/notte-core/src/notte_core/credentials/bitwarden.py` around lines 95 - 96, Several async methods (notably _get_credentials_impl and other async vault paths) call the synchronous helpers _fetch_secrets and _run_bws directly, which blocks the event loop; update these async methods to call those blocking functions via asyncio.to_thread(...) (e.g., replace direct calls to _fetch_secrets() and _run_bws(...) inside _get_credentials_impl and the other async methods with await asyncio.to_thread(self._fetch_secrets) or await asyncio.to_thread(self._run_bws, ...)) so CLI work runs in a thread and does not stall unrelated coroutines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/notte-core/src/notte_core/credentials/bitwarden.py`:
- Around line 111-113: The current code serializes credentials into value and
passes them as a positional arg to bws via self._run_bws (the "secret create"
call), which exposes secrets to process inspectors; change the implementation to
avoid argv exposure by writing the serialized secret to a temporary file with
restrictive permissions (owner-only), pass the temp file path or read it into
bws if supported, ensure the temp file is securely deleted after use, and update
any callers of the self._run_bws("secret", "create", ...) invocation accordingly
(or, if you opt to document the limitation instead, add a clear comment in
bitwarden.py near the value/self._run_bws usage explaining the risk and
recommended deployment mitigations); leave self._secrets_cache handling
unchanged.
---
Duplicate comments:
In `@packages/notte-core/src/notte_core/credentials/bitwarden.py`:
- Around line 95-96: Several async methods (notably _get_credentials_impl and
other async vault paths) call the synchronous helpers _fetch_secrets and
_run_bws directly, which blocks the event loop; update these async methods to
call those blocking functions via asyncio.to_thread(...) (e.g., replace direct
calls to _fetch_secrets() and _run_bws(...) inside _get_credentials_impl and the
other async methods with await asyncio.to_thread(self._fetch_secrets) or await
asyncio.to_thread(self._run_bws, ...)) so CLI work runs in a thread and does not
stall unrelated coroutines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd09c9b1-f880-4687-a035-9ef924268ed8
📒 Files selected for processing (1)
packages/notte-core/src/notte_core/credentials/bitwarden.py
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Install aac + bws CLIs in the CI workflow - Remove AAC_INTEGRATION_TEST env gate — tests run whenever aac is available - Add real-binary integration tests: CLI validation, error handling, invalid token rejection - BWS tests requiring a token are still gated behind BWS_ACCESS_TOKEN Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/integration/test_aac_vault_integration.py (2)
26-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure
vault.stop()always runs viatry/finally.If the async call or assertion fails, cleanup is skipped and cached session/process state can leak across integration runs. This issue was previously flagged and remains unaddressed.
♻️ Proposed fix
def test_aac_vault_list_credentials_returns_empty() -> None: vault = AacVault(token=None) vault.start() - creds = asyncio.run(vault.list_credentials_async()) - assert creds == [] - vault.stop() + try: + creds = asyncio.run(vault.list_credentials_async()) + assert creds == [] + finally: + vault.stop()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_aac_vault_integration.py` around lines 26 - 31, The test currently may skip cleanup if the async call or assertion fails; wrap the runtime and assertion in a try/finally so AacVault.stop() always runs: after calling vault.start() in test_aac_vault_list_credentials_returns_empty, place asyncio.run(vault.list_credentials_async()) and the assert inside a try block and call vault.stop() in the finally block to guarantee cleanup for the AacVault instance.
34-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure
vault.stop()always runs viatry/finally.If the async call or assertion fails, cleanup is skipped. This test follows the same unsafe pattern as the others.
♻️ Proposed fix
def test_aac_vault_get_credentials_returns_none_without_session() -> None: """Without a paired session, credential requests should return None gracefully.""" vault = AacVault(token=None, timeout=5) vault.start() - creds = asyncio.run(vault.get_credentials_async("https://example.com")) - assert creds is None - vault.stop() + try: + creds = asyncio.run(vault.get_credentials_async("https://example.com")) + assert creds is None + finally: + vault.stop()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_aac_vault_integration.py` around lines 34 - 40, The test test_aac_vault_get_credentials_returns_none_without_session must ensure AacVault.stop() always runs; wrap the async call and assertion in a try/finally so vault.stop() is invoked in the finally block. Specifically, keep creating vault = AacVault(token=None, timeout=5) and vault.start(), then perform creds = asyncio.run(vault.get_credentials_async(...)) and the assert inside the try, and call vault.stop() in the finally; reference AacVault.start, AacVault.stop, and get_credentials_async to locate the code to update.
🧹 Nitpick comments (2)
.github/workflows/test-cicd.yml (2)
111-116: ⚡ Quick winAdd checksum verification for the bws CLI download.
While pinning to
v1.0.0ensures reproducibility, the installation lacks checksum verification. If the downloaded archive is corrupted or tampered with, it will be installed without validation. Consider adding SHA256 checksum verification for security.🔒 Proposed fix to add checksum verification
- name: Install bws CLI (Bitwarden Secrets Manager) run: | curl -sL "https://github.com/bitwarden/sdk/releases/download/bws-v1.0.0/bws-x86_64-unknown-linux-gnu-1.0.0.zip" -o /tmp/bws.zip + echo "<expected-sha256-checksum> /tmp/bws.zip" | sha256sum -c - unzip -o /tmp/bws.zip -d /tmp/bws sudo mv /tmp/bws/bws /usr/local/bin/ bws --version🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test-cicd.yml around lines 111 - 116, The "Install bws CLI (Bitwarden Secrets Manager)" step downloads bws-x86_64-unknown-linux-gnu-1.0.0.zip without validating integrity; add SHA256 checksum verification after download and before unzip by obtaining or embedding the expected SHA256 value, computing the checksum of /tmp/bws.zip (e.g., via sha256sum) and failing the job if it doesn't match, only proceeding to unzip/move the binary when the checksum verification succeeds; update the step that runs the curl/unzip/mv commands to include the checksum check and a clear error exit on mismatch.
105-109: ⚡ Quick winPin the aac CLI version for reproducible builds.
Using
latestintroduces non-determinism and may break CI unexpectedly if a new aac release has breaking changes. Thebwsinstallation (lines 111-116) pins tov1.0.0, butaacdoes not. Consider pinning both to specific versions and verifying checksums for security and reproducibility.🔒 Proposed fix to pin version and add checksum verification
- - name: Install aac CLI (Agent Access Protocol) - run: | - curl -sL https://github.com/bitwarden/agent-access/releases/latest/download/aac-linux-x86_64.tar.gz | tar xz - sudo mv aac /usr/local/bin/ - aac --version + - name: Install aac CLI (Agent Access Protocol) + run: | + AAC_VERSION="v0.1.0" # Replace with actual pinned version + curl -sL "https://github.com/bitwarden/agent-access/releases/download/${AAC_VERSION}/aac-linux-x86_64.tar.gz" -o /tmp/aac.tar.gz + echo "<expected-sha256-checksum> /tmp/aac.tar.gz" | sha256sum -c - + tar xzf /tmp/aac.tar.gz + sudo mv aac /usr/local/bin/ + aac --version🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test-cicd.yml around lines 105 - 109, The workflow installs the aac CLI from the `latest` URL which makes CI non-deterministic; update the "Install aac CLI (Agent Access Protocol)" step to download a specific release tag instead of `latest` (mirror how `bws` is pinned), e.g. replace the download URL used by the current `curl | tar` with the versioned release asset for the chosen tag, and add a checksum verification (e.g. download the published SHA256 and compare with sha256sum) before moving the binary to /usr/local/bin/ and running `aac --version` to ensure reproducible and secure builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/test_aac_vault_integration.py`:
- Around line 19-23: Wrap the test body in a try/finally so vault.stop() always
runs (e.g., vault.start(); try: ...; finally: vault.stop()) and replace the
private-attribute assertion on vault._paired with an assertion against a public
observable (call a public method/property such as AacVault.is_paired() or
AacVault.paired); if no public accessor exists, add a simple is_paired() method
on AacVault that returns self._paired and assert against that instead so the
test doesn’t rely on internals.
---
Duplicate comments:
In `@tests/integration/test_aac_vault_integration.py`:
- Around line 26-31: The test currently may skip cleanup if the async call or
assertion fails; wrap the runtime and assertion in a try/finally so
AacVault.stop() always runs: after calling vault.start() in
test_aac_vault_list_credentials_returns_empty, place
asyncio.run(vault.list_credentials_async()) and the assert inside a try block
and call vault.stop() in the finally block to guarantee cleanup for the AacVault
instance.
- Around line 34-40: The test
test_aac_vault_get_credentials_returns_none_without_session must ensure
AacVault.stop() always runs; wrap the async call and assertion in a try/finally
so vault.stop() is invoked in the finally block. Specifically, keep creating
vault = AacVault(token=None, timeout=5) and vault.start(), then perform creds =
asyncio.run(vault.get_credentials_async(...)) and the assert inside the try, and
call vault.stop() in the finally; reference AacVault.start, AacVault.stop, and
get_credentials_async to locate the code to update.
---
Nitpick comments:
In @.github/workflows/test-cicd.yml:
- Around line 111-116: The "Install bws CLI (Bitwarden Secrets Manager)" step
downloads bws-x86_64-unknown-linux-gnu-1.0.0.zip without validating integrity;
add SHA256 checksum verification after download and before unzip by obtaining or
embedding the expected SHA256 value, computing the checksum of /tmp/bws.zip
(e.g., via sha256sum) and failing the job if it doesn't match, only proceeding
to unzip/move the binary when the checksum verification succeeds; update the
step that runs the curl/unzip/mv commands to include the checksum check and a
clear error exit on mismatch.
- Around line 105-109: The workflow installs the aac CLI from the `latest` URL
which makes CI non-deterministic; update the "Install aac CLI (Agent Access
Protocol)" step to download a specific release tag instead of `latest` (mirror
how `bws` is pinned), e.g. replace the download URL used by the current `curl |
tar` with the versioned release asset for the chosen tag, and add a checksum
verification (e.g. download the published SHA256 and compare with sha256sum)
before moving the binary to /usr/local/bin/ and running `aac --version` to
ensure reproducible and secure builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1350976-635f-438b-b25f-af251adf4500
📒 Files selected for processing (3)
.github/workflows/test-cicd.ymltests/integration/test_aac_vault_integration.pytests/integration/test_bitwarden_vault_integration.py
Comment on lines
+19
to
+23
| def test_aac_vault_starts_without_token() -> None: | ||
| vault = AacVault(token=None) | ||
| vault.start() | ||
| assert vault._paired is False | ||
| vault.stop() |
Contributor
There was a problem hiding this comment.
Ensure cleanup with try/finally and avoid accessing private attributes.
If start() or the assertion fails, stop() won't execute, potentially leaving stale state for subsequent tests. Additionally, testing vault._paired exposes implementation details; consider testing observable behavior instead.
♻️ Proposed fix
def test_aac_vault_starts_without_token() -> None:
vault = AacVault(token=None)
vault.start()
- assert vault._paired is False
- vault.stop()
+ try:
+ # Test observable behavior instead of private state
+ creds = asyncio.run(vault.list_credentials_async())
+ assert creds == [] # Unpaired vault should return empty list
+ finally:
+ vault.stop()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/test_aac_vault_integration.py` around lines 19 - 23, Wrap
the test body in a try/finally so vault.stop() always runs (e.g., vault.start();
try: ...; finally: vault.stop()) and replace the private-attribute assertion on
vault._paired with an assertion against a public observable (call a public
method/property such as AacVault.is_paired() or AacVault.paired); if no public
accessor exists, add a simple is_paired() method on AacVault that returns
self._paired and assert against that instead so the test doesn’t rely on
internals.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Needs attention — 2 issues across 2 files
The pyright CI failure (reportMissingTypeStubs = false causing reportUnnecessaryTypeIgnoreComment warnings) is fixed in bde17e0. The two blocking-in-async bugs flagged in every prior review round remain unaddressed: _request_credential in aac.py and _fetch_secrets/SDK calls in bitwarden.py are still called synchronously from async methods, blocking the event loop for the full network/subprocess round-trip.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<assessment>
The pyright CI failure (`reportMissingTypeStubs = false` causing `reportUnnecessaryTypeIgnoreComment` warnings) is fixed in `bde17e0`. The two blocking-in-async bugs flagged in every prior review round remain unaddressed: `_request_credential` in `aac.py` and `_fetch_secrets`/SDK calls in `bitwarden.py` are still called synchronously from `async` methods, blocking the event loop for the full network/subprocess round-trip.
</assessment>
<file name="packages/notte-core/src/notte_core/credentials/aac.py">
<issue location="packages/notte-core/src/notte_core/credentials/aac.py:142">
Blocking `subprocess.run` (via `_request_credential`) called synchronously inside an `async` method stalls the event loop for up to `_timeout` seconds. Use `asyncio.to_thread`.
</issue>
</file>
<file name="packages/notte-core/src/notte_core/credentials/bitwarden.py">
<issue location="packages/notte-core/src/notte_core/credentials/bitwarden.py:133">
Blocking SDK network calls (via `_fetch_secrets`) called synchronously inside an `async` method stall the event loop for the full round-trip. Use `asyncio.to_thread`.
</issue>
</file>
Tag @mendral-app with feedback or questions. View session
Comment on lines
+142
to
+162
| async def _get_credentials_impl(self, url: str) -> CredentialsDict | None: | ||
| domain = get_root_domain(url) | ||
| if not domain: | ||
| return None | ||
|
|
||
| cred = self._request_credential(domain) | ||
| if cred is None: | ||
| return None | ||
|
|
||
| password = cred.get("password") | ||
| if not password: | ||
| return None | ||
|
|
||
| result: CredentialsDict = {"password": password} | ||
| username = cred.get("username") | ||
| if username: | ||
| result["username"] = username | ||
| totp = cred.get("totp") | ||
| if totp: | ||
| result["mfa_secret"] = totp | ||
| return result |
There was a problem hiding this comment.
bug (P1): Blocking subprocess.run (via _request_credential) called synchronously inside an async method stalls the event loop for up to _timeout seconds. Use asyncio.to_thread.
Suggested change
Suggested change
| async def _get_credentials_impl(self, url: str) -> CredentialsDict | None: | |
| domain = get_root_domain(url) | |
| if not domain: | |
| return None | |
| cred = self._request_credential(domain) | |
| if cred is None: | |
| return None | |
| password = cred.get("password") | |
| if not password: | |
| return None | |
| result: CredentialsDict = {"password": password} | |
| username = cred.get("username") | |
| if username: | |
| result["username"] = username | |
| totp = cred.get("totp") | |
| if totp: | |
| result["mfa_secret"] = totp | |
| return result | |
| @override | |
| async def _get_credentials_impl(self, url: str) -> CredentialsDict | None: | |
| import asyncio | |
| domain = get_root_domain(url) | |
| if not domain: | |
| return None | |
| cred = await asyncio.to_thread(self._request_credential, domain) | |
| if cred is None: | |
| return None | |
| password = cred.get("password") | |
| if not password: | |
| return None | |
| result: CredentialsDict = {"password": password} | |
| username = cred.get("username") | |
| if username: | |
| result["username"] = username | |
| totp = cred.get("totp") | |
| if totp: | |
| result["mfa_secret"] = totp | |
| return result |
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/notte-core/src/notte_core/credentials/aac.py, line 142:
<issue>
Blocking `subprocess.run` (via `_request_credential`) called synchronously inside an `async` method stalls the event loop for up to `_timeout` seconds. Use `asyncio.to_thread`.
</issue>
Comment on lines
+133
to
+143
| async def _get_credentials_impl(self, url: str) -> CredentialsDict | None: | ||
| secrets = self._secrets_cache if self._secrets_cache is not None else self._fetch_secrets() | ||
| target_domain = get_root_domain(url) | ||
| for secret in secrets: | ||
| parsed = self._parse_secret(secret) | ||
| if parsed is None: | ||
| continue | ||
| secret_url, creds = parsed | ||
| if get_root_domain(secret_url) == target_domain: | ||
| return creds | ||
| return None |
There was a problem hiding this comment.
bug (P1): Blocking SDK network calls (via _fetch_secrets) called synchronously inside an async method stall the event loop for the full round-trip. Use asyncio.to_thread.
Suggested change
Suggested change
| async def _get_credentials_impl(self, url: str) -> CredentialsDict | None: | |
| secrets = self._secrets_cache if self._secrets_cache is not None else self._fetch_secrets() | |
| target_domain = get_root_domain(url) | |
| for secret in secrets: | |
| parsed = self._parse_secret(secret) | |
| if parsed is None: | |
| continue | |
| secret_url, creds = parsed | |
| if get_root_domain(secret_url) == target_domain: | |
| return creds | |
| return None | |
| @override | |
| async def _get_credentials_impl(self, url: str) -> CredentialsDict | None: | |
| import asyncio | |
| secrets = self._secrets_cache if self._secrets_cache is not None else await asyncio.to_thread(self._fetch_secrets) | |
| target_domain = get_root_domain(url) | |
| for secret in secrets: | |
| parsed = self._parse_secret(secret) | |
| if parsed is None: | |
| continue | |
| secret_url, creds = parsed | |
| if get_root_domain(secret_url) == target_domain: | |
| return creds | |
| return None |
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/notte-core/src/notte_core/credentials/bitwarden.py, line 133:
<issue>
Blocking SDK network calls (via `_fetch_secrets`) called synchronously inside an `async` method stall the event loop for the full round-trip. Use `asyncio.to_thread`.
</issue>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BitwardenVault— wraps thebwsCLI to fetch credentials from Bitwarden Secrets Manager. Bulk-loads secrets onstart(), matches by root domain viaget_root_domain(). Users store credentials as JSON secrets:{"url": "...", "password": "...", "username": "...", "email": "...", "mfa_secret": "..."}.AacVault— wraps theaacCLI (Agent Access Protocol) to fetch credentials on-demand through an E2E encrypted Noise tunnel. Works with any aac-compatible provider (Bitwarden Password Manager, etc.). Overridesget_credentials_async()to bypass TOTP generation since aac returns live codes.Both are drop-in replacements for
NotteVaultin local agent mode — they implementBaseVault+SyncResourceand work with the existing placeholder/masking system.No new dependencies — both use CLI subprocesses (
bws,aac), no Rust build or PyO3 bindings needed.Usage
Test plan
aac listensession🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Examples
Tests
Chores
Note
Adds two new
BaseVaultimplementations:BitwardenVault(Bitwarden Python SDK, bulk-loads secrets onstart()) andAacVault(wrapsaacCLI, fetches credentials on-demand via E2E Noise tunnel). Both implementBaseVault + SyncResourceas drop-in replacements forNotteVault. The latest commit (bde17e0) fixes the previous CI blocker by switching from the globalreportMissingTypeStubs = falsepyright flag to per-import inline ignores.Written by Mendral for commit bde17e0.