fix: infer vlt org from coordinate owner - #5
Conversation
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughThe client now infers accessible organization owners for ChangesOwner-aware secret resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant readSecret
participant secretsApiForOwner
participant getConfigForVltOwner
participant rawSecretsApiWithConfig
readSecret->>secretsApiForOwner: pass reference owner and secret path
secretsApiForOwner->>getConfigForVltOwner: resolve owner configuration
getConfigForOwner-->>secretsApiForOwner: return VaultConfig
secretsApiForOwner->>rawSecretsApiWithConfig: request secret with VaultConfig
rawSecretsApiWithConfig-->>readSecret: return secret response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
codeant-ai
Bot
added
the
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
node/src/api.test.ts (1)
282-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover in-flight probe deduplication.
Lines 291-301 await the first resolution before starting the second resolution. This verifies completed-result caching only. Start both resolutions before awaiting either one. Assert that one status request occurs for the normalized owner.
Proposed test update
- expect(await getConfigForVltOwner("CirclesAC")).toEqual({ + const [first, second] = await Promise.all([ + getConfigForVltOwner("CirclesAC"), + getConfigForVltOwner("circlesac"), + ]) + expect(first).toEqual({ baseUrl: "https://vault.circles.ac/circlesac", token, org: "circlesac", }) - expect(await getConfigForVltOwner("circlesac")).toEqual({ + expect(second).toEqual({ baseUrl: "https://vault.circles.ac/circlesac", token, org: "circlesac",🤖 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 `@node/src/api.test.ts` around lines 282 - 301, Update the test around getConfigForVltOwner to start both owner lookups concurrently before awaiting either result, then assert both return the expected normalized configuration and fetchSpy records exactly one status request. This must verify in-flight probe deduplication rather than only completed-result caching.
🤖 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.
Nitpick comments:
In `@node/src/api.test.ts`:
- Around line 282-301: Update the test around getConfigForVltOwner to start both
owner lookups concurrently before awaiting either result, then assert both
return the expected normalized configuration and fetchSpy records exactly one
status request. This must verify in-flight probe deduplication rather than only
completed-result caching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a619fdfd-bc45-4774-8759-8a103682cd05
📒 Files selected for processing (5)
README.mdcli/src/index.tsnode/src/api.test.tsnode/src/api.tsnode/src/cli.ts
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| async function canAccessOrg(config: VaultConfig, org: string): Promise<boolean> { | ||
| const key = `${config.baseUrl}\n${org}` | ||
| let probe = orgAccessProbes.get(key) | ||
| if (!probe) { |
There was a problem hiding this comment.
Suggestion: The probe cache key omits the bearer token and has no expiry. If credentials are refreshed, rotated, or permissions change while the process remains alive, a result obtained with the old token is reused: a stale false result forces an accessible organization to personal scope, while a stale true result continues selecting an organization after access is revoked. Include the credential identity/token validity in the cache key or invalidate probes whenever the resolved credential changes, and add an expiry policy. [cache]
Severity Level: Major ⚠️
- ⚠️ Long-lived SDK reads can use stale organization scope.
- ❌ Rotated credentials may fail owner-specific secret lookups.
- ⚠️ Revoked membership remains selected until process restart.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** node/src/api.ts
**Line:** 161:164
**Comment:**
*Cache: The probe cache key omits the bearer token and has no expiry. If credentials are refreshed, rotated, or permissions change while the process remains alive, a result obtained with the old token is reused: a stale false result forces an accessible organization to personal scope, while a stale true result continues selecting an organization after access is revoked. Include the credential identity/token validity in the cache key or invalidate probes whenever the resolved credential changes, and add an expiry policy.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| export async function secretsApiForOwner<T = unknown>( | ||
| owner: string, | ||
| path: string, | ||
| opts: { method?: string; body?: unknown } = {} | ||
| ): Promise<T> { | ||
| try { | ||
| return await rawSecretsApiWithConfig<T>(await getConfigForVltOwner(owner), path, opts) |
There was a problem hiding this comment.
Suggestion: The new owner-aware helper is used by the CLI, but the SDK's createVaultClient().read() path still calls rawSecretsApi, which resolves only getConfig() and therefore reads vlt:// references from the personal account. Consumers using the SDK receive different behavior from the CLI and can get a personal-account lookup or failure instead of the owner's organization secret. Update the SDK read path to parse the owner and use the owner-aware request path. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ SDK `read()` ignores `vlt://` organization owners.
- ❌ Personal lookup can fail for organization secrets.
- ⚠️ CLI and SDK produce inconsistent reference behavior.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** node/src/api.ts
**Line:** 231:237
**Comment:**
*Incomplete Implementation: The new owner-aware helper is used by the CLI, but the SDK's `createVaultClient().read()` path still calls `rawSecretsApi`, which resolves only `getConfig()` and therefore reads `vlt://` references from the personal account. Consumers using the SDK receive different behavior from the CLI and can get a personal-account lookup or failure instead of the owner's organization secret. Update the SDK read path to parse the owner and use the owner-aware request path.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix1 participant
User description
Refs circlesac/vault-service#13
Changes
vlt://github.com/<owner>/...reference.read,run, andinject.vlt://exception to the general personal-by-default account rule.Testing
bun test node/src/ cli/src/— 73 pass, 1 platform-specific skipnpm run build:sdkbun run buildcvlt readwithout--orgreturns the same value as explicit--org circlesaccvlt run --env-fileandcvlt injectresolve owner-global references without--org--org meltenis rejected before readingCodeAnt-AI Description
Infer the organization for
vlt://references from their ownerWhat Changed
vlt://github.com/<owner>/...reads now use the owner’s organization when the current credential can access it, without requiring--orgvlt://owner and now fail clearly when they do notrunandinjectresolve each owner independently, while repeated checks reuse the same access resultvlt://organization inference exceptionImpact
✅ Organization secrets resolve without manually selecting --org✅ Personal fallback for inaccessible or personal owners✅ Clear errors for mismatched organization selections💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
vlt://secret references can now infer an accessible owner organization automatically.runandinject.Documentation