Conversation
WalkthroughThis PR introduces a local file-based credential vault service for development and self-hosted deployments. It adds configuration settings, a new LocalCredentialVaultService class for managing credentials in JSON files with atomic writes and locking, integrates it into the ForgeApp system, and extends the credential routing layer to support the LOCAL vault type. Changes
Sequence DiagramsequenceDiagram
actor User
participant API as API Request
participant Router as Credential Router
participant Service as LocalCredentialVaultService
participant FileSystem as JSON File Store
participant Database as Credentials DB
User->>API: Create/Retrieve Credential
API->>Router: Route to vault service
Router->>Service: Get vault service (type=LOCAL)
rect rgba(100, 150, 200, 0.5)
Note over Service,FileSystem: Create Credential Flow
Service->>FileSystem: Acquire lock & read store
Service->>FileSystem: Add credential entry (with generated item_id)
Service->>FileSystem: Write store atomically (temp + rename)
Service->>Database: Create credential record
alt Database fails
Service->>FileSystem: Rollback store deletion
end
end
rect rgba(150, 200, 150, 0.5)
Note over Service,FileSystem: Retrieve Credential Flow
Service->>FileSystem: Acquire lock & read store
Service->>FileSystem: Lookup credential by item_id
Service->>Service: Deserialize credential data
Service-->>API: Return mapped CredentialItem
end
API-->>User: Credential response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 73933ac in 45 seconds. Click for details.
- Reviewed
393lines of code in5files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_kDWgAcq0LZnWxRfX
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@skyvern/forge/sdk/services/credential/local_credential_vault_service.py`:
- Around line 36-41: The _PasswordCredentialData model currently lacks
totp_identifier and totp_type fields causing loss of that data when persisting
PasswordCredential; add totp_identifier: str | None = None and totp_type: str |
None = None to the _PasswordCredentialData BaseModel and update the
serialization/deserialization logic where PasswordCredential instances are
converted to/from _PasswordCredentialData (the block that writes/reads totp) to
include totp_identifier and totp_type so they are persisted and restored
alongside totp.
- Around line 201-212: Wrap the rollback that removes the credential from local
storage in its own try-except so failures in self._write_store don't silently
leave orphaned credentials; inside the except, log a warning/error via LOG
(including organization_id, item_id and the caught exception) and consider
recording the failure for later cleanup, then re-raise the original exception
from the outer block (or attach both errors) so callers still see the DB
creation error; update the block around async with self._lock / store = await
self._read_store() / store.credentials.pop(...) / await self._write_store(store)
to implement this handling.
🧹 Nitpick comments (3)
skyvern/forge/sdk/services/credential/local_credential_vault_service.py (3)
14-14: Potential circular import withappmodule.Importing
appat module level may cause circular import issues sinceforge_app.pyimports this service. Consider importingappinside the methods that need it, or accessing it through a parameter.♻️ Proposed refactor to avoid circular import
-from skyvern.forge import appThen, in the methods that need
app, import it locally:async def delete_credential(self, credential: Credential) -> None: from skyvern.forge import app # ... rest of the method
231-239: Consider reversing deletion order for better consistency.Deleting from DB first (line 232) means if the local file operation fails, the credential is lost from DB but remains in local storage. Reversing the order (local first, then DB) would be safer—orphaned DB records are easier to detect and clean up than orphaned local entries.
♻️ Proposed refactor to delete local first
async def delete_credential(self, credential: Credential) -> None: """Delete a credential from the local file and database.""" LOG.info( "Deleting credential from local storage", organization_id=credential.organization_id, credential_id=credential.credential_id, item_id=credential.item_id, ) - # Delete from database first - await app.DATABASE.delete_credential(credential.credential_id, credential.organization_id) - # Delete from local storage async with self._lock: store = await self._read_store() if credential.item_id in store.credentials: del store.credentials[credential.item_id] await self._write_store(store) + # Delete from database after local storage + from skyvern.forge import app + await app.DATABASE.delete_credential(credential.credential_id, credential.organization_id) + LOG.info( "Successfully deleted credential from local storage", organization_id=credential.organization_id, credential_id=credential.credential_id, )
106-111: UseLOG.exceptioninstead ofLOG.errorin exception handlers.Per static analysis (Ruff TRY400), using
LOG.exceptionautomatically includes the traceback, which aids debugging file I/O issues. This applies to lines 107, 110, and 130.♻️ Proposed fix
except json.JSONDecodeError as e: - LOG.error("Failed to parse local credential file", error=str(e), file_path=str(self._file_path)) + LOG.exception("Failed to parse local credential file", file_path=str(self._file_path)) raise ValueError(f"Invalid JSON in credential file: {e}") from e except Exception as e: - LOG.error("Failed to read local credential file", error=str(e), file_path=str(self._file_path)) + LOG.exception("Failed to read local credential file", file_path=str(self._file_path)) raise ValueError(f"Failed to read credential file: {e}") from e
| class _PasswordCredentialData(BaseModel): | ||
| type: Literal["password"] | ||
| username: str | ||
| password: str | ||
| totp: str | None = None | ||
|
|
There was a problem hiding this comment.
Missing totp_identifier and totp_type in local storage model.
The _PasswordCredentialData model stores totp but omits totp_identifier and totp_type fields that exist on PasswordCredential. When credentials are retrieved, totp_type comes from the DB record, but totp_identifier is never persisted to the local file and will be lost.
🔧 Proposed fix to include missing fields
class _PasswordCredentialData(BaseModel):
type: Literal["password"]
username: str
password: str
totp: str | None = None
+ totp_type: str = "none"
+ totp_identifier: str | None = NoneAnd update serialization at line 146:
return self._PasswordCredentialData(
type="password",
username=credential.username,
password=credential.password,
totp=credential.totp,
+ totp_type=credential.totp_type.value if hasattr(credential, 'totp_type') else "none",
+ totp_identifier=credential.totp_identifier if hasattr(credential, 'totp_identifier') else None,
).model_dump()🤖 Prompt for AI Agents
In `@skyvern/forge/sdk/services/credential/local_credential_vault_service.py`
around lines 36 - 41, The _PasswordCredentialData model currently lacks
totp_identifier and totp_type fields causing loss of that data when persisting
PasswordCredential; add totp_identifier: str | None = None and totp_type: str |
None = None to the _PasswordCredentialData BaseModel and update the
serialization/deserialization logic where PasswordCredential instances are
converted to/from _PasswordCredentialData (the block that writes/reads totp) to
include totp_identifier and totp_type so they are persisted and restored
alongside totp.
| except Exception: | ||
| # Rollback: remove from local storage | ||
| LOG.warning( | ||
| "DB creation failed, rolling back local storage", | ||
| organization_id=organization_id, | ||
| item_id=item_id, | ||
| ) | ||
| async with self._lock: | ||
| store = await self._read_store() | ||
| store.credentials.pop(item_id, None) | ||
| await self._write_store(store) | ||
| raise |
There was a problem hiding this comment.
Rollback may leave orphaned credentials in local storage.
If the rollback _write_store call at line 211 fails (e.g., disk full, permissions), the credential remains in local storage without a corresponding DB record. Consider wrapping the rollback in a try-except and logging the failure so it can be cleaned up later.
🛡️ Proposed fix to handle rollback failures
except Exception:
# Rollback: remove from local storage
LOG.warning(
"DB creation failed, rolling back local storage",
organization_id=organization_id,
item_id=item_id,
)
- async with self._lock:
- store = await self._read_store()
- store.credentials.pop(item_id, None)
- await self._write_store(store)
+ try:
+ async with self._lock:
+ store = await self._read_store()
+ store.credentials.pop(item_id, None)
+ await self._write_store(store)
+ except Exception as rollback_error:
+ LOG.exception(
+ "Failed to rollback local storage after DB failure, orphaned credential may exist",
+ organization_id=organization_id,
+ item_id=item_id,
+ rollback_error=str(rollback_error),
+ )
raise📝 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.
| except Exception: | |
| # Rollback: remove from local storage | |
| LOG.warning( | |
| "DB creation failed, rolling back local storage", | |
| organization_id=organization_id, | |
| item_id=item_id, | |
| ) | |
| async with self._lock: | |
| store = await self._read_store() | |
| store.credentials.pop(item_id, None) | |
| await self._write_store(store) | |
| raise | |
| except Exception: | |
| # Rollback: remove from local storage | |
| LOG.warning( | |
| "DB creation failed, rolling back local storage", | |
| organization_id=organization_id, | |
| item_id=item_id, | |
| ) | |
| try: | |
| async with self._lock: | |
| store = await self._read_store() | |
| store.credentials.pop(item_id, None) | |
| await self._write_store(store) | |
| except Exception as rollback_error: | |
| LOG.exception( | |
| "Failed to rollback local storage after DB failure, orphaned credential may exist", | |
| organization_id=organization_id, | |
| item_id=item_id, | |
| rollback_error=str(rollback_error), | |
| ) | |
| raise |
🤖 Prompt for AI Agents
In `@skyvern/forge/sdk/services/credential/local_credential_vault_service.py`
around lines 201 - 212, Wrap the rollback that removes the credential from local
storage in its own try-except so failures in self._write_store don't silently
leave orphaned credentials; inside the except, log a warning/error via LOG
(including organization_id, item_id and the caught exception) and consider
recording the failure for later cleanup, then re-raise the original exception
from the outer block (or attach both errors) so callers still see the DB
creation error; update the block around async with self._lock / store = await
self._read_store() / store.credentials.pop(...) / await self._write_store(store)
to implement this handling.
1 participant
🔧 This PR introduces local file-based credential storage support for Skyvern, enabling development, testing, and hobby self-hosting scenarios by storing credentials in a local JSON file as an alternative to cloud-based vault services.
🔍 Detailed Analysis
Key Changes
LOCALas a newCredentialVaultTypeenum value alongside existing Bitwarden, Azure, and Custom optionsLocalCredentialVaultServicethat stores credentials in a configurable JSON file (.skyvern_credentials.jsonby default)LOCAL_CREDENTIAL_FILEsetting to specify the credential storage file pathTechnical Implementation
sequenceDiagram participant Client participant API participant LocalService participant FileSystem participant Database Client->>API: Create Credential Request API->>LocalService: create_credential() LocalService->>FileSystem: Read existing store LocalService->>FileSystem: Write updated store LocalService->>Database: Create DB record LocalService-->>API: Return Credential API-->>Client: Credential Response Client->>API: Get Credential Request API->>Database: Fetch credential metadata API->>LocalService: get_credential_item() LocalService->>FileSystem: Read credential data LocalService-->>API: Return CredentialItem API-->>Client: Full credential dataImpact
Created with Palmier
Important
Adds local credential storage support via
LocalCredentialVaultServicefor development and testing, with configuration and integration updates.LocalCredentialVaultServiceinlocal_credential_vault_service.pyfor local file-based credential storage.password,credit_card, andsecretcredential types.LOCAL_CREDENTIAL_FILEsetting inconfig.pyfor specifying local credential file path.CredentialVaultTypeinschemas/credentials.pyto includeLOCAL.create_forge_app()inforge_app.pyto initializeLocalCredentialVaultService._get_credential_vault_service()inroutes/credentials.pyto handleLOCALvault type.This description was created by
for 73933ac. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
✏️ Tip: You can customize this high-level summary in your review settings.