fix(cli): derive Blob storeId from token to prevent URL drift#64
Open
znat wants to merge 2 commits into
Open
Conversation
VercelBlobStorage previously computed the public URL host from the storeId in .gitpulse.json, while uploads went to whichever store the BLOB_READ_WRITE_TOKEN encoded. When the two diverged (token rotated to a different store, or config copy-pasted from a sibling project), the SDK's `put()` succeeded but the imageUrl baked into release/story JSON pointed at a host with no file — so the analyzer reported `image:stored` while every imageUrl 404'd. - Derive the storeId from the token; keep `storeId` in config optional and only as a sanity assertion (throw on mismatch). - Pass through the URL `put()` returns instead of synthesizing one from config. Eliminates the divergence at the source. - On the next analyzer run, treat any existing imageUrl whose host doesn't match the storage's current host as missing, so the 8 stale release imageUrls regenerate against the correct store. - Drop the now-redundant storeId from this repo's .gitpulse.json. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR updates Vercel Blob storage to make ChangesVercel Blob Storage Optional storeId and Image URL Stability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
Contributor
|
Vercel preview — built from |
|
| Filename | Overview |
|---|---|
| cli/src/image/storage/vercel-blob.ts | Core fix: storeSlug now derived from the token via storeIdFromToken(); mismatch with opts.storeId throws early; upload() returns put()'s URL; new ownsUrl() method checks leftmost subdomain. |
| cli/src/index.ts | Backfill block clears existingImageUrl when ownsUrl() returns false, triggering image regen for stale releases. No equivalent backfill for feature stories — existing story JSONs with stale imageUrl values will persist since processCommit only runs on commits not in seenSha. |
| cli/src/image/storage/vercel-blob.test.ts | Good new test coverage: storeIdFromToken extraction, mismatch detection, case-insensitive storeId comparison, upload URL passthrough, and ownsUrl positive/negative cases. |
| cli/src/image/storage/types.ts | upload() return type changed from Promise to Promise; ownsUrl() added to interface; storeId made optional in StorageConfig discriminated union — all consistent with the implementation changes. |
| cli/src/image/generate-feature-image.ts | Simplified to return the URL directly from storage.upload(), eliminating the redundant urlFor() call that caused the original URL drift bug. |
| cli/src/image/generate-release-image.ts | Same cleanup as generate-feature-image.ts — returns URL from upload() directly instead of resynthesizing via urlFor(). |
| cli/src/project-config.ts | storeId made optional in the Zod schema to match the updated TypeScript types; schema validation passes without storeId now. |
| cli/src/image/storage/index.ts | createStorage now passes an empty options object when storeId is absent, letting the constructor derive it from the token. |
| .gitpulse.json | Removed storeId from the repo's own config — the field that encoded the wrong store ID and was the direct cause of the URL drift bug. |
Sequence Diagram
sequenceDiagram
participant Analyzer as Analyzer (index.ts)
participant VBS as VercelBlobStorage
participant SDK as @vercel/blob SDK
participant Store as Vercel Blob Store
Note over VBS: Constructor: derive storeSlug from BLOB_READ_WRITE_TOKEN<br/>Optionally assert opts.storeId matches → throw if not
Analyzer->>VBS: upload(key, buffer, mime)
VBS->>SDK: put(key, buffer, opts)
SDK->>Store: PUT blob
Store-->>SDK: "{url}"
SDK-->>VBS: "{url}"
VBS-->>Analyzer: url (SDK-canonical, not synthesized)
Note over Analyzer: Backfill check (releases only)
Analyzer->>VBS: ownsUrl(existingImageUrl)
VBS-->>Analyzer: false (stale store)
Note over Analyzer: Clear existingImageUrl → triggers regen
Analyzer->>VBS: upload(key, newBuffer, mime)
VBS-->>Analyzer: correct url
Reviews (2): Last reviewed commit: "fix(cli): address PR #64 review comments" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 2
🤖 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 `@cli/src/image/storage/vercel-blob.ts`:
- Around line 24-32: The error message uses raw tokenStoreId and opts.storeId
which can look different even though comparison used normalizeStoreId; update
the thrown Error in the block that compares want/got (where want =
normalizeStoreId(opts.storeId) and got = normalizeStoreId(tokenStoreId)) to
include the normalized values (want and got) in the message so both are shown in
the same format (e.g., `... points to store "${got}" but .gitpulse.json declares
storeId "${want}" ...`), making the mismatch clear.
- Around line 89-101: The regex in storeIdFromToken is too restrictive (only
alphanumeric) and can reject valid Vercel blob tokens; update the pattern used
in storeIdFromToken to capture any characters up to the next underscore (e.g.
use a pattern that matches /^vercel_blob_rw_([^_]+)_/ on token.trim()) so store
IDs with non-alphanumeric characters are accepted, keep the existing error
handling and the subsequent normalizeStoreId()/host derivation logic unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1197b3ec-7c60-4a9f-985d-a6fc89f24a58
📒 Files selected for processing (9)
.gitpulse.jsoncli/src/image/generate-feature-image.tscli/src/image/generate-release-image.tscli/src/image/storage/index.tscli/src/image/storage/types.tscli/src/image/storage/vercel-blob.test.tscli/src/image/storage/vercel-blob.tscli/src/index.tscli/src/project-config.ts
- Relax storeIdFromToken regex from [A-Za-z0-9]+ to [^_]+. Vercel doesn't document an allowed-character whitelist for storeIds, so the parser now only commits to the underscore separator between the three token parts. Hypothetical tokens with hyphens or other characters no longer throw at startup. (greptile / coderabbit) - Replace the host-equality staleness check with a semantic storage.ownsUrl(url) method on the ImageStorage interface. The vercel-blob implementation matches by storeId (leftmost subdomain label) rather than full host, so a future change in Vercel's URL shape doesn't false-trigger image regeneration each cycle while still catching real cross-store drift. (greptile) - Normalize the storeId mismatch error message so both values appear in the same format. (coderabbit nit) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cli/src/image/storage/vercel-blob.test.ts`:
- Around line 60-65: Add a test that verifies dotted storeIds round-trip through
the storage constructor and ownership check: call the same factory used
elsewhere to build a Vercel blob storage instance from a dotted token (use the
token pattern 'vercel_blob_rw_abc.def_secret' and the same helper used in other
tests), then assert that storage.ownsUrl(storage.url) (or the equivalent method
used in this test suite) returns true; reference the existing helpers
storeIdFromToken and ownsUrl (and the storage factory used in other tests) so
the new case mirrors the hyphen test but ensures dotted IDs pass the end-to-end
ownership contract.
In `@cli/src/image/storage/vercel-blob.ts`:
- Around line 84-88: ownsUrl currently only compares the first host label to
this.storeSlug, which breaks when store IDs (from storeIdFromToken) can contain
dots (e.g., "abc.def"); update ownsUrl to split both the host and this.storeSlug
on '.' and compare the first N host labels (where N =
this.storeSlug.split('.').length) joined with '.' to this.storeSlug so
multi-label store IDs are correctly matched; keep the existing try/catch and URL
parsing logic and only change the comparison logic in ownsUrl (referencing
ownsUrl and this.storeSlug).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5372010e-c5a6-4c19-bf14-2ed8a6f3db6c
📒 Files selected for processing (4)
cli/src/image/storage/types.tscli/src/image/storage/vercel-blob.test.tscli/src/image/storage/vercel-blob.tscli/src/index.ts
Comment on lines
+60
to
+65
| it('accepts non-alphanumeric characters in the storeId (e.g. hyphens)', () => { | ||
| // Vercel doesn't document an allowed-character whitelist for storeIds, | ||
| // so the parser must not assume they're alphanumeric-only. | ||
| expect(storeIdFromToken('vercel_blob_rw_Ab-Cd-Ef_secret')).toBe('Ab-Cd-Ef'); | ||
| expect(storeIdFromToken('vercel_blob_rw_abc.def_secret')).toBe('abc.def'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a dotted-storeId round-trip test for ownsUrl().
Since Line 60-65 explicitly allows dotted store IDs, add a case where storage is built from a dotted token and ownsUrl() returns true for that same store URL. This guards the parser/ownership contract end-to-end.
Suggested test addition
describe('VercelBlobStorage', () => {
+ it('ownsUrl: supports dotted storeIds accepted by token parser', () => {
+ const storage = new VercelBlobStorage(
+ {},
+ { BLOB_READ_WRITE_TOKEN: 'vercel_blob_rw_abc.def_secret' },
+ );
+ expect(storage.ownsUrl('https://abc.def.public.blob.vercel-storage.com/releases/v1.jpg')).toBe(true);
+ });Also applies to: 165-182
🤖 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 `@cli/src/image/storage/vercel-blob.test.ts` around lines 60 - 65, Add a test
that verifies dotted storeIds round-trip through the storage constructor and
ownership check: call the same factory used elsewhere to build a Vercel blob
storage instance from a dotted token (use the token pattern
'vercel_blob_rw_abc.def_secret' and the same helper used in other tests), then
assert that storage.ownsUrl(storage.url) (or the equivalent method used in this
test suite) returns true; reference the existing helpers storeIdFromToken and
ownsUrl (and the storage factory used in other tests) so the new case mirrors
the hyphen test but ensures dotted IDs pass the end-to-end ownership contract.
Comment on lines
+84
to
+88
| ownsUrl(url: string): boolean { | ||
| try { | ||
| const host = new URL(url).host; | ||
| const leftmost = host.split('.')[0]?.toLowerCase() ?? ''; | ||
| return leftmost === this.storeSlug; |
Contributor
There was a problem hiding this comment.
ownsUrl() is incompatible with store IDs that storeIdFromToken() now accepts.
Line 122 accepts store IDs like abc.def, but Line 87 only compares the first host label (abc). That makes same-store URLs fail ownership checks for accepted token formats.
Suggested fix
ownsUrl(url: string): boolean {
try {
- const host = new URL(url).host;
- const leftmost = host.split('.')[0]?.toLowerCase() ?? '';
- return leftmost === this.storeSlug;
+ const hostname = new URL(url).hostname.toLowerCase();
+ return hostname === this.storeSlug || hostname.startsWith(`${this.storeSlug}.`);
} catch {
return false;
}
}Also applies to: 122-123
🤖 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 `@cli/src/image/storage/vercel-blob.ts` around lines 84 - 88, ownsUrl currently
only compares the first host label to this.storeSlug, which breaks when store
IDs (from storeIdFromToken) can contain dots (e.g., "abc.def"); update ownsUrl
to split both the host and this.storeSlug on '.' and compare the first N host
labels (where N = this.storeSlug.split('.').length) joined with '.' to
this.storeSlug so multi-label store IDs are correctly matched; keep the existing
try/catch and URL parsing logic and only change the comparison logic in ownsUrl
(referencing ownsUrl and this.storeSlug).
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
1 participant
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
VercelBlobStoragecomputed the public URL host from thestoreIdin.gitpulse.json, whileput()always wrote to whichever store theBLOB_READ_WRITE_TOKENencodes. When the two diverged, uploads succeeded but theimageUrlbaked into release/story JSON pointed at a different host with no file — the analyzer happily loggedimage:storedwhile every URL 404'd.storeIdis now derived from the token; the config field stays as an optional sanity assertion (mismatch → throw with a clear message).upload()returns the URLput()reports, so callers never resynthesize one.generate-release-imageandgenerate-feature-imagenow hand that URL straight through.imageUrlwhose host doesn't match the storage's current host is treated as missing — the 8 stale release JSONs in this repo (v0.1.0…v0.2.0) will regenerate against the correct store with one-time Gemini calls. After that, the cache hash keeps things cheap.storeIdfield from this repo's.gitpulse.json.Why the bug bit us
Two stores existed in the linked Vercel account:
store_jrSOlxo1lISQ2owg— what.gitpulse.json(and my local.env.local) declared.store_ryXhp9N7MsNEbMh1— what the GitHubBLOB_READ_WRITE_TOKENsecret encodes.Result: CI uploads landed in
ryX…, the analyzer wrote URLs pointing atjrS…(empty), every releaseimageUrl404'd. Local dev wouldn't catch this because the local token matched the local config.After this PR, that whole class of mismatch becomes impossible without surfacing — either the URL
put()returns is the canonical one, or startup throws.Test plan
yarn vitest run— 111 passed (new tests cover token parsing, mismatch detection, and theput()-returned URL contract)yarn workspace @gitpulse/cli build— clean.gitpulse.json(nostoreId)/data/releases/v0.2.0.jsonimageUrlresolves with HTTP 200.env.localtoken to match the same store as the GitHub secret (or vice versa — your call)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores