refactor(credentials): lean credentials command surface - #334
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 · |
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
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
PR Summary by QodoRefactor credentials command surface; preserve failure tags and add validation tests
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| selector: string, | ||
| operation: string, | ||
| ): Effect.Effect<AccountRecord, CredentialsCommandFailure> => { | ||
| const matchedAccount = matchAccount([...accounts], selector); |
There was a problem hiding this comment.
Suggestion: matchAccount resolves each record by checking its label immediately after its own Key ID, so a label collision can shadow a later account's Key ID. For example, if account A has label B and account B has Key ID B, selecting B resolves account A; this helper now applies that ambiguity to use, rename, remove, refresh, and explicit account selection. Resolve exact Key ID matches globally before falling back to labels, or reject ambiguous selectors. [incorrect condition logic]
Severity Level: Critical 🚨
- ❌ `creds remove` can delete the wrong signing account.
- ❌ `creds rename` can alter the wrong account label.
- ⚠️ `creds use` can activate the wrong Apple credentials.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/credentials/command.ts
**Line:** 139:139
**Comment:**
*Incorrect Condition Logic: `matchAccount` resolves each record by checking its label immediately after its own Key ID, so a label collision can shadow a later account's Key ID. For example, if account A has label `B` and account B has Key ID `B`, selecting `B` resolves account A; this helper now applies that ambiguity to `use`, `rename`, `remove`, `refresh`, and explicit account selection. Resolve exact Key ID matches globally before falling back to labels, or reject ambiguous selectors.
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| const accounts = yield* listAccounts(); | ||
| const conflictingAccount = accounts.find( | ||
| (account) => | ||
| account.label.toLowerCase() === accountLabel.toLowerCase() && account.keyId !== keyId, | ||
| ); | ||
| if (conflictingAccount !== undefined) { | ||
| return yield* failCommand( | ||
| 'select account label', | ||
| `Label "${accountLabel}" is already used by key ${conflictingAccount.keyId}.`, | ||
| ); | ||
| } | ||
| yield* requireUniqueAccountLabel(accounts, accountLabel, keyId, 'select account label'); |
There was a problem hiding this comment.
Suggestion: The uniqueness check is a non-atomic read-before-write. Concurrent set-key invocations can both observe that the label is unused and proceed to addAccount, allowing duplicate labels or causing one registry write to overwrite the other account. Enforce the label invariant within the account update or serialize the check and write. [race condition]
Severity Level: Major ⚠️
- ❌ Concurrent imports can create duplicate account labels.
- ❌ Registry writes can lose a concurrently imported account.
- ⚠️ Subsequent selectors may resolve an unintended account.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/credentials/command.ts
**Line:** 341:342
**Comment:**
*Race Condition: The uniqueness check is a non-atomic read-before-write. Concurrent `set-key` invocations can both observe that the label is unused and proceed to `addAccount`, allowing duplicate labels or causing one registry write to overwrite the other account. Enforce the label invariant within the account update or serialize the check and write.
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
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTo customize comments, go to the Qodo configuration screen, or learn more in the docs. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Why: Dead CLI Promise facades and re-wrapped command failures hid the thin Commander/core boundary for `launch creds`. What: Drop unused chooseAccountInteractive/setupIos CLI facades and redundant Apple adapter provides; preserve CredentialsCommandFailure operations; DRY matched-account and unique-label checks; add pure validation tests; refresh docs test count. Impact: Behavior preserved; thinner CLI; actionable failure operations reach callers; gate green.
9a4a2e5 to
bde1f74
Compare
1 participant
Fixes #319
Summary
Lean remediation of the credentials command surface (
src/core/credentials/command.ts+ thinsrc/cli/commands/creds.ts).chooseAccountInteractive/setupIosPromise facades (wizard already imports core) and drop redundantAppStoreIdentityLive/AppleCredentialsClientLiveprovides already applied byrunCliProgram— aligns with open refactor(cli): drop unused CLI compatibility facades #305 facade-delete intent without owning that PR’s other files.CredentialsCommandFailuretags throughmapErrorso operation-specific messages (rename/remove/use/push-key) are not re-wrapped asrun credentials command.requireMatchedAccountandrequireUniqueAccountLabelreplace five copy-pasted match/conflict blocks (no file split — no second callers outside this module, path globs forbid other files).ApnsKeyRecordre-export from the command module.Scope
src/core/credentials/command.tssrc/core/credentials/command.test.tssrc/cli/commands/creds.tssrc/cli/commands/creds.test.tsREADME*.md,docs/commands.md,llms.txt) required fordocs:checkRisk
Low. Behavior preserved; no ASC/Play transport changes. CLI exports shrink — only
registerCredsCommandremains (nothing imported the removed facades).Test plan
pnpm typecheckpnpm lintpnpm lint:stylepnpm docs:checkpnpm test(2092 passed)pnpm buildMain safety
origin/main@a57cdb1Summary by cubic
Leaned the
launch credsCLI and refactored core credential handling to cut duplication and keep operation-specific error messages intact. Behavior is unchanged; the CLI surface is smaller and failures are clearer.Refactors
chooseAccountInteractiveandsetupIosfromsrc/cli/commands/creds.ts;registerCredsCommandis the only export.AppStoreIdentityLiveandAppleCredentialsClientLiveprovides;runCliProgramalready wires adapters.requireMatchedAccountandrequireUniqueAccountLabelhelpers to de-duplicate match/conflict checks.ApnsKeyRecordre-export.logoutalias acceptance, and the thin-CLI export.docs/commands.md.Bug Fixes
CredentialsCommandFailuretags by passing through existing failures inmapError, so specific operations aren’t re-wrapped as “run credentials command”.Written for commit 086ea4a. Summary will update on new commits.