Conversation
PR SummaryMedium Risk Overview The script supports a dry-run audit (detects conflicts, previews inserts, writes a workspace-id list for a follow-up live run) and a live mode that encrypts selected keys (including resolving Written by Cursor Bugbot for commit 7f6eb10. This will update automatically on new commits. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR adds a self-contained Bun migration script ( Key observations:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([Start]) --> B{dry-run flag?}
B -- Yes --> C[Query distinct workspace IDs from DB]
B -- No --> D[Read workspace IDs from --from-file]
C --> E[Init migrate-byok-workspace-ids.txt]
E --> F[Process workspaces in batches of 1000 concurrency 100]
D --> F
F --> G[Fetch matching blocks and workspace owner]
G --> H{providerKeys found?}
H -- No --> I[Skip workspace]
H -- Yes --> J{Env var references present?}
J -- Yes --> K[Fetch workspace_environment and personal environment]
J -- No --> L[Resolve keys]
K --> L
L --> M{resolved.length > 0?}
M -- No --> N[Continue to next provider]
M -- Yes --> O[Sort by KEY_SOURCE_PRIORITY plaintext=0 workspace=1 personal=2]
O --> P{distinct keys > 1?}
P -- Yes --> Q[Log CONFLICT and pick resolved index 0]
P -- No --> R[Pick resolved index 0]
Q --> S{DRY_RUN?}
R --> S
S -- Yes --> T[Log preview and write workspace ID to file]
S -- No --> U[Encrypt chosen key and INSERT with onConflictDoNothing]
U --> V{Rows returned?}
V -- Yes --> W[stats.inserted++]
V -- No --> X[stats.skippedExisting++]
T --> Y[Next provider]
W --> Y
X --> Y
I --> Z[Next workspace]
N --> Y
Y --> Z
Z --> AA{More batches?}
AA -- Yes --> AB[Sleep SLEEP_MS between batches]
AB --> F
AA -- No --> AC([Print summary and exit])
Last reviewed commit: 399db36 |
| const subBlocks = block.subBlocks as Record<string, { value?: any }> | ||
|
|
||
| const providerId = BLOCK_TYPE_TO_PROVIDER[block.blockType] | ||
| if (providerId) { | ||
| const val = subBlocks?.apiKey?.value | ||
| if (typeof val === 'string' && val.trim()) { | ||
| const refs = providerKeys.get(providerId) ?? [] |
There was a problem hiding this comment.
index parameter shadows drizzle-orm import
The processWorkspace function's index parameter shadows the index named import from drizzle-orm/pg-core (used in the table definitions at module scope). While this doesn't cause a runtime error (the module-level index is only needed at definition time), it creates a confusing naming collision. Consider renaming the parameter to avoid the shadow:
| const subBlocks = block.subBlocks as Record<string, { value?: any }> | |
| const providerId = BLOCK_TYPE_TO_PROVIDER[block.blockType] | |
| if (providerId) { | |
| const val = subBlocks?.apiKey?.value | |
| if (typeof val === 'string' && val.trim()) { | |
| const refs = providerKeys.get(providerId) ?? [] | |
| async function processWorkspace( | |
| workspaceId: string, | |
| allBlockTypes: string[], | |
| userFilter: ReturnType<typeof sql>, | |
| total: number, | |
| workspaceIndex: number | |
| ): Promise<WorkspaceResult> { |
Then update all references to index inside the function to workspaceIndex.
|
|
||
| // ---------- DB ---------- | ||
| const postgresClient = postgres(CONNECTION_STRING, { |
There was a problem hiding this comment.
SLEEP_MS value contradicts the code comment
SLEEP_MS is set to 30_000 (30 seconds), but the processing loop (line ~620) has a comment that says "pausing for 60s after each 1000". One of the two needs to be corrected to avoid confusion for operators running this script.
| // ---------- DB ---------- | |
| const postgresClient = postgres(CONNECTION_STRING, { | |
| const SLEEP_MS = 60_000 |
| continue | ||
| } | ||
|
|
||
| try { | ||
| const encrypted = await encryptSecret(chosen.key) |
There was a problem hiding this comment.
Dry-run writes workspace IDs even when all keys fail to resolve
shouldWriteWorkspaceId is set to DRY_RUN (i.e., always true in dry-run mode) for any workspace that has at least one matching block. However, if every API key reference for a workspace fails to resolve — for example because all values are {{ENV_VAR}} references that don't exist in the environment tables — resolved.length === 0 for every provider and the continue is hit each time. The workspace ID is still written to migrate-byok-workspace-ids.txt.
When the live run later reads that file, it processes the workspace, finds no insertable keys, and does nothing — wasting a DB round-trip per failed workspace. More importantly, the dry-run summary message ("Wrote ${stats.workspacesProcessed} workspace IDs (with keys)") will report a higher count than the number of IDs that will actually produce inserts.
Consider tracking a per-workspace boolean that is only set to true when at least one resolved.length > 0 path is reached:
let hasInsertableKey = false
// ... inside the provider loop, before `continue`:
if (resolved.length === 0) continue
hasInsertableKey = true
// ...
return { stats, shouldWriteWorkspaceId: DRY_RUN && hasInsertableKey }
|
|
||
| function extractEnvVarName(value: string): string | null { | ||
| const match = ENV_VAR_PATTERN.exec(value) | ||
| return match ? match[1].trim() : null | ||
| } | ||
|
|
There was a problem hiding this comment.
Priority ordering and log message are inconsistent
KEY_SOURCE_PRIORITY assigns plaintext = 0, workspace = 1, personal = 2, and the sort at line ~503 is ascending — so resolved[0] is the entry with the lowest priority number (i.e., a bare plaintext key in a block wins over a workspace env-var reference). The conflict log on line ~514 says "Using highest-priority key", which contradicts the ascending sort (it actually picks the lowest-numbered entry).
This is at minimum a confusing log message. More importantly, it's worth verifying the intended precedence: should a workspace-level env var key take precedence over a plaintext block key, or the other way around? If a workspace env var should win, the priority values should be inverted (plaintext: 2, workspace: 1, personal: 0) or the sort order should be descending.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
* Add byok migration script * Fix lint * Add skipping if byok already provided * Fix lint --------- Co-authored-by: Theodore Li <theo@sim.ai>
Summary
Adding hosted keys will overwrite anyone's currently existing api keys. Added migration script to move existing api keys to byok so our users don't get auto-switched over.
Fixes #(issue)
Type of Change
Testing
Ran in staging and local, validated that byok is migrated over.
Checklist
Screenshots/Videos