Add vault secrets and prompt variable support#1725
Add vault secrets and prompt variable support#1725AviVolah wants to merge 2 commits intopingdotgg:mainfrom
Conversation
- Store named secrets in encrypted desktop vaults - Expose vault management in settings and inject variables into provider prompts - Add revert controls for turn checkpoints # Conflicts: # apps/desktop/src/main.ts # packages/contracts/src/settings.ts
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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-actions
bot
added
size:XXL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| }); | ||
| } | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
🟢 Low src/secretVault.ts:261
The catch block at line 261 swallows the specific error thrown at line 242 for unsupported vault file versions, replacing it with the generic "Secret vault data is unreadable." message. Users lose the diagnostic information that their vault file needs migration. Consider rethrowing version errors directly or including the original error message in the wrapped error.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/desktop/src/secretVault.ts around line 261:
The `catch` block at line 261 swallows the specific error thrown at line 242 for unsupported vault file versions, replacing it with the generic "Secret vault data is unreadable." message. Users lose the diagnostic information that their vault file needs migration. Consider rethrowing version errors directly or including the original error message in the wrapped error.
Evidence trail:
apps/desktop/src/secretVault.ts lines 232-262 at REVIEWED_COMMIT:
- Line 232: try block starts
- Line 241: `throw new Error("Unsupported vault file version.");` thrown when version check fails
- Line 260: `} catch {` blanket catch block
- Line 261: `throw new Error("Secret vault data is unreadable.");` replaces any caught error
| private ensureLoaded(): void { | ||
| if (this.loaded) { | ||
| return; | ||
| } | ||
| this.loaded = true; | ||
|
|
||
| const candidatePaths = [ | ||
| this.options.vaultPath, | ||
| ...(this.options.legacyVaultPaths ?? []), | ||
| ].filter((candidatePath, index, paths) => paths.indexOf(candidatePath) === index); | ||
|
|
||
| const existingVaultPath = candidatePaths.find((candidatePath) => FS.existsSync(candidatePath)); | ||
| if (!existingVaultPath) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const raw = FS.readFileSync(existingVaultPath, "utf8"); | ||
| const parsed = JSON.parse(raw) as Partial<SecretVaultFile>; | ||
| if ( | ||
| parsed.version !== 1 && | ||
| parsed.version !== 2 && | ||
| parsed.version !== 3 && | ||
| parsed.version !== SECRET_VAULT_VERSION | ||
| ) { | ||
| throw new Error("Unsupported vault file version."); | ||
| } | ||
|
|
||
| if ( | ||
| (parsed.version === 2 || parsed.version === 3 || parsed.version === SECRET_VAULT_VERSION) && | ||
| parsed.namedSecrets | ||
| ) { | ||
| for (const [secretId, record] of Object.entries(parsed.namedSecrets)) { | ||
| if (typeof secretId !== "string" || !isNamedSecretVaultRecord(record)) { | ||
| continue; | ||
| } | ||
|
|
||
| this.namedSecrets.set(secretId as VaultSecretId, { | ||
| key: record.key, | ||
| ciphertext: record.ciphertext, | ||
| updatedAt: record.updatedAt, | ||
| }); | ||
| } | ||
| } | ||
| } catch { | ||
| throw new Error("Secret vault data is unreadable."); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 High src/secretVault.ts:217
In ensureLoaded(), this.loaded = true is set before the try block, so if JSON parsing or validation throws, subsequent calls skip loading and operate on an empty namedSecrets map. This causes data loss: saveNamedSecret() can overwrite the original vault file with only new secrets. Move this.loaded = true to after successful loading, or use a separate flag to distinguish "loading attempted" from "loaded successfully".
private ensureLoaded(): void {
if (this.loaded) {
return;
}
- this.loaded = true;
const candidatePaths = [
this.options.vaultPath,
...(this.options.legacyVaultPaths ?? []),
@@ -228,6 +227,7 @@ export class SecretVault {
}
try {
+ this.loaded = true;
const raw = FS.readFileSync(existingVaultPath, "utf8");🤖 Copy this AI Prompt to have your agent fix this:
In file apps/desktop/src/secretVault.ts around lines 217-264:
In `ensureLoaded()`, `this.loaded = true` is set before the try block, so if JSON parsing or validation throws, subsequent calls skip loading and operate on an empty `namedSecrets` map. This causes data loss: `saveNamedSecret()` can overwrite the original vault file with only new secrets. Move `this.loaded = true` to after successful loading, or use a separate flag to distinguish "loading attempted" from "loaded successfully".
Evidence trail:
apps/desktop/src/secretVault.ts lines 217-264 (REVIEWED_COMMIT): `ensureLoaded()` method showing `this.loaded = true` at line 221 before try block at line 232. apps/desktop/src/secretVault.ts lines 139-177 (REVIEWED_COMMIT): `saveNamedSecret()` calls `ensureLoaded()` at line 141, then `persist()` at line 172. apps/desktop/src/secretVault.ts lines 266-276 (REVIEWED_COMMIT): `persist()` writes `namedSecrets` map to disk, overwriting the vault file.
| clear: (input) => getRpcClient().terminal.clear(input as never), | ||
| restart: (input) => getRpcClient().terminal.restart(input as never), | ||
| close: (input) => getRpcClient().terminal.close(input as never), | ||
| onEvent: (callback) => getRpcClient().terminal.onEvent(callback), |
There was a problem hiding this comment.
🟡 Medium src/wsNativeApi.ts:42
terminal.onEvent and orchestration.onDomainEvent register subscriptions on a specific WsRpcClient instance, but getWsRpcClient() creates and switches to a new client when the WebSocket URL changes. The old client is disposed, so existing callbacks stop receiving events permanently even though the code still references them. Consider re-registering subscriptions on the new client when the URL changes, or documenting that callers must re-subscribe after URL changes.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/web/src/wsNativeApi.ts around line 42:
`terminal.onEvent` and `orchestration.onDomainEvent` register subscriptions on a specific `WsRpcClient` instance, but `getWsRpcClient()` creates and switches to a new client when the WebSocket URL changes. The old client is disposed, so existing callbacks stop receiving events permanently even though the code still references them. Consider re-registering subscriptions on the new client when the URL changes, or documenting that callers must re-subscribe after URL changes.
Evidence trail:
apps/web/src/wsNativeApi.ts lines 41-42, 119 (subscription methods call getRpcClient()); apps/web/src/wsRpcClient.ts lines 98-108 (getWsRpcClient creates new client on URL change and disposes old one); apps/web/src/wsTransport.ts lines 73-107 (subscribe method), lines 117-125 (dispose method sets this.disposed=true causing subscription interruption)
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. This PR adds a significant new feature: vault secrets with encrypted desktop storage and vault variables that are automatically injected into model prompts. The changes span multiple layers (desktop, web, server, contracts) and introduce new user-facing UI, new storage mechanisms, and modify prompt behavior in model adapters—warranting human review for a feature of this scope. You can customize Macroscope's approvability policy. Learn more. |
1 participant
Summary
Validation
Note
Add vault secrets and prompt variable support to settings and AI adapters
SecretVaultclass in the Electron main process that stores named secrets encrypted viasafeStorage, with IPC endpoints (desktop:vault:list-secrets,desktop:vault:save-secret,desktop:vault:delete-secret) and push broadcasts to all windows.window.desktopBridgeand adds avaultnamespace toNativeApi, wiring them through a new/settings/secretsroute that rendersVaultSettingsPanel.VaultVariabletoServerSettingsand injects variables into user prompts viainjectVaultVariablesIntoPromptin bothClaudeAdapterandCodexAdapterbefore sending to the model.getWsRpcClientto recreate the RPC client when the WebSocket URL changes and updatescreateWsNativeApito callgetWsRpcClient()on each API call instead of capturing a snapshot at construction time.Revertbutton inMessagesTimelinedriven by newrevertTurnCountByTurnIdandonRevertTurnprops passed fromChatView.BASE_DIR/userdatatoBASE_DIR/dev, which may affect existing dev environment state.📊 Macroscope summarized fad054d. 25 files reviewed, 4 issues evaluated, 0 issues filtered, 3 comments posted
🗂️ Filtered Issues