Add pluggable storage backends for CacheStorage#1566
Add pluggable storage backends for CacheStorage#1566hoverlover wants to merge 7 commits intobrowserbase:mainfrom
Conversation
Introduces a StorageAdapter interface that allows CacheStorage to use different storage backends. This enables cache persistence in environments without mounted filesystems (Trigger.dev, serverless). New adapters: - FilesystemAdapter: Default filesystem storage (extracted from CacheStorage) - NullAdapter: No-op adapter when caching is disabled - InMemoryAdapter: For testing with error injection support - GCSAdapter: Google Cloud Storage for cloud/serverless environments The refactor is backward compatible - existing `cacheDir` string API still works. New `cacheAdapter` option accepts custom StorageAdapter implementations. Includes: - Contract tests for all adapter implementations - Backward compatibility tests for CacheStorage - GCS adapter unit tests (integration tests gated by env vars) - Updated documentation with usage examples Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Accept credentials as object (GCSCredentials interface) - Accept credentials as JSON string (for serverless environments) - Accept credentials as file path (for local development) - Fall back to ADC if no credentials provided - Export GCSCredentials type from adapters index Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Greptile SummaryIntroduced a well-architected pluggable storage adapter system for CacheStorage, enabling flexible cache backends beyond the default filesystem. The implementation maintains full backward compatibility while adding support for cloud storage (GCS) for serverless environments. Key changes:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant V3 as Stagehand (V3)
participant CacheStorage
participant Adapter as StorageAdapter
participant GCS as GCS/Filesystem
Note over User,GCS: Initialization Flow
User->>V3: new Stagehand({ cacheAdapter: new GCSAdapter() })
V3->>CacheStorage: CacheStorage.create(cacheDir, logger, { adapter })
alt Custom adapter provided
CacheStorage->>CacheStorage: Use provided adapter
else cacheDir provided (backward compat)
CacheStorage->>Adapter: FilesystemAdapter.create(cacheDir)
Adapter-->>CacheStorage: FilesystemAdapter instance
else No cache configured
CacheStorage->>Adapter: new NullAdapter()
end
CacheStorage-->>V3: CacheStorage instance
Note over User,GCS: Cache Write Flow
User->>V3: act("click button")
V3->>CacheStorage: writeJson(key, actionData)
CacheStorage->>Adapter: writeJson(key, data)
alt GCSAdapter
Adapter->>Adapter: getClient() - lazy init
Adapter->>GCS: Storage.bucket().file().save()
GCS-->>Adapter: Success/Error
else FilesystemAdapter
Adapter->>GCS: fs.writeFile()
GCS-->>Adapter: Success/Error
end
Adapter-->>CacheStorage: { } or { error }
CacheStorage-->>V3: WriteJsonResult
Note over User,GCS: Cache Read Flow
User->>V3: act("click button") [same instruction]
V3->>CacheStorage: readJson(key)
CacheStorage->>Adapter: readJson(key)
alt GCSAdapter
Adapter->>Adapter: getClient() - reuse cached client
Adapter->>GCS: Storage.bucket().file().download()
alt File exists
GCS-->>Adapter: File contents
Adapter-->>CacheStorage: { value: parsedData }
else File not found (404)
GCS-->>Adapter: 404 error
Adapter-->>CacheStorage: { value: null }
end
else FilesystemAdapter
Adapter->>GCS: fs.readFile()
alt File exists
GCS-->>Adapter: File contents
Adapter-->>CacheStorage: { value: parsedData }
else ENOENT
GCS-->>Adapter: ENOENT error
Adapter-->>CacheStorage: { value: null }
end
end
CacheStorage-->>V3: ReadJsonResult
V3-->>User: Cached action or execute new
|
Greptile's behavior is changing!From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
There was a problem hiding this comment.
4 issues found across 16 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/docs/v3/best-practices/caching.mdx">
<violation number="1" location="packages/docs/v3/best-practices/caching.mdx:206">
P2: GCSAdapter docs use unsupported `keyFilename` option; adapter only accepts `credentials` (string/object/path), so example won’t type-check or apply credentials.</violation>
</file>
<file name="packages/core/tests/cache/adapters/StorageAdapter.contract.test.ts">
<violation number="1" location="packages/core/tests/cache/adapters/StorageAdapter.contract.test.ts:310">
P2: FilesystemAdapter failure test is permission-dependent; creating `/nonexistent-root-12345/cache` can succeed under root, making the test flaky across environments.</violation>
</file>
<file name="packages/core/lib/v3/cache/adapters/FilesystemAdapter.ts">
<violation number="1" location="packages/core/lib/v3/cache/adapters/FilesystemAdapter.ts:43">
P1: FilesystemAdapter allows directory traversal/absolute path escape because cache keys are joined without validation</violation>
</file>
<file name="packages/core/tests/cache/CacheStorage.backward-compat.test.ts">
<violation number="1" location="packages/core/tests/cache/CacheStorage.backward-compat.test.ts:51">
P2: Test assumes directory creation under `/nonexistent-root-12345/cache` will fail; with root/sufficient permissions `fs.mkdirSync` succeeds, making the test flaky and not reliably testing the failure path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/core/tests/cache/adapters/StorageAdapter.contract.test.ts
Outdated
Show resolved
Hide resolved
- Add path traversal protection in FilesystemAdapter - Fix docs to use `credentials` instead of deprecated `keyFilename` - Fix flaky tests using null byte paths instead of permission-dependent paths - Add tests for path traversal attack prevention - Increase timeout for GCS error handling test Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 16 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/lib/v3/cache/adapters/FilesystemAdapter.ts">
<violation number="1" location="packages/core/lib/v3/cache/adapters/FilesystemAdapter.ts:49">
P2: Path traversal check breaks when cacheDir is the filesystem root, rejecting all keys and disabling caching in that configuration.</violation>
</file>
<file name="packages/core/tests/cache/adapters/GCSAdapter.test.ts">
<violation number="1" location="packages/core/tests/cache/adapters/GCSAdapter.test.ts:94">
P2: Unit test performs real GCS calls without mocking/gating, causing environment-dependent behavior and potential timeouts</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Fix FilesystemAdapter path traversal check when cacheDir is root (/) - Replace real GCS network calls with mocked tests for error handling - Add test for 404 handling (file not found returns null, not error) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 16 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/tests/cache/adapters/GCSAdapter.test.ts">
<violation number="1" location="packages/core/tests/cache/adapters/GCSAdapter.test.ts:111">
P2: Mocked @google-cloud/storage is left in the module cache; unmocking without reset causes later tests (including integration) to run against the cached mock instead of the real client</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Add afterEach hook to properly reset the module cache after each mocked test. This ensures integration tests use the real GCS client instead of the cached mock. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 16 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/lib/v3/cache/adapters/FilesystemAdapter.ts">
<violation number="1" location="packages/core/lib/v3/cache/adapters/FilesystemAdapter.ts:50">
P2: Path traversal guard rejects all keys when cacheDir is a Windows drive root because the constructed prefix adds an extra separator (e.g., "C:\\\\"), so resolved paths like "C:\\file" fail the startsWith check.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The path traversal guard was incorrectly rejecting all valid cache keys when cacheDir was a Windows drive root like C:\. The bug was that the original check `this.dir === path.sep` returned false for "C:\" !== "\", causing the prefix to become "C:\\" which broke the startsWith check. Fix: Use path.parse() to correctly identify filesystem roots on both Unix (/) and Windows (C:\) platforms. When parsed.root === this.dir, we know it's a root and don't need to append an extra separator. Adds tests for root path detection and prefix construction logic. Fixes: cubic review issue on PR browserbase#1566 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 16 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/tests/cache/adapters/StorageAdapter.contract.test.ts">
<violation number="1" location="packages/core/tests/cache/adapters/StorageAdapter.contract.test.ts:431">
P2: Windows drive root test never uses FilesystemAdapter, so the regression isn’t actually tested</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The test was mocking the path module but never actually creating or using a FilesystemAdapter instance - it just verified path.parse behavior directly. Now the test: - Mocks both path and fs modules for Windows behavior - Actually creates a FilesystemAdapter with "C:\" as the cache directory - Calls readJson to exercise the resolvePath logic - Verifies that valid keys are accepted without path traversal errors Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
This PR introduces a pluggable storage adapter system for CacheStorage, enabling flexible cache backends beyond the default filesystem.
New Storage Adapters
readJson/writeJsonmethodsGCSAdapter Features
@google-cloud/storageto avoid bundling when not usedConfiguration
Backward Compatibility
cacheDiroption continues to workcacheAdapteroption takes precedence when both are specifiedTest plan
RUN_GCS_INTEGRATION_TESTS=true)🤖 Generated with Claude Code
Summary by cubic
Adds pluggable storage adapters to CacheStorage so caching works beyond the local filesystem, including a GCS backend for serverless/cloud. Existing cacheDir usage is preserved; a new cacheAdapter option enables custom backends.
New Features
Bug Fixes
Dependencies
Written for commit 3b6b219. Summary will update on new commits.