refactor(credentials): readonly-domain-types (stack 7/12, re-split #307) - #380
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 · |
codeant-ai
Bot
added
the
size:M
PR Summary by Qodorefactor(credentials): adapt consumers to readonly domain types
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
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. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| }; | ||
| if (input.teamId !== null && input.teamId !== undefined) record.teamId = input.teamId; | ||
| if (input.apps !== undefined && input.apps.length > 0) record.apps = input.apps; | ||
| if (input.apps !== undefined && input.apps.length > 0) record.apps = [...input.apps]; |
There was a problem hiding this comment.
Suggestion: When an existing account is re-imported after identity resolution returns an empty app list—for example, a transient Apple lookup failure—the rebuilt record omits apps instead of preserving the previously cached list. This silently discards usable account metadata and can make a previously resolved account appear unresolved; preserve the existing list when the new identity result is unavailable, or distinguish a successful empty result from a failed lookup. [stale reference]
Severity Level: Major ⚠️
- ⚠️ Account summaries lose previously cached application names.
- ⚠️ Credential status and account pickers lose useful metadata.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/credentials/accounts.ts
**Line:** 216:216
**Comment:**
*Stale Reference: When an existing account is re-imported after identity resolution returns an empty app list—for example, a transient Apple lookup failure—the rebuilt record omits `apps` instead of preserving the previously cached list. This silently discards usable account metadata and can make a previously resolved account appear unresolved; preserve the existing list when the new identity result is unavailable, or distinguish a successful empty result from a failed lookup.
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| let next: AccountRecord = { ...account, resolvedAt: timestamp }; | ||
| if (teamId != null) next = { ...next, teamId }; | ||
| if (apps.length > 0) next = { ...next, apps: [...apps] }; |
There was a problem hiding this comment.
Suggestion: An identity refresh with apps = [] advances resolvedAt but leaves the old app list untouched. Since the identity service converts app-list lookup failures to an empty array, revoked access or a failed lookup can permanently display stale applications while marking the account as freshly resolved. Replace the cached list on a successful refresh, and separately represent lookup failure if stale data must be retained. [cache]
Severity Level: Major ⚠️
- ⚠️ Credential refresh can display stale application access.
- ⚠️ `resolvedAt` falsely indicates current identity data.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/credentials/accounts.ts
**Line:** 239:241
**Comment:**
*Cache: An identity refresh with `apps = []` advances `resolvedAt` but leaves the old app list untouched. Since the identity service converts app-list lookup failures to an empty array, revoked access or a failed lookup can permanently display stale applications while marking the account as freshly resolved. Replace the cached list on a successful refresh, and separately represent lookup failure if stale data must be retained.
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| keyId: string, | ||
| bundleId: string, | ||
| extensions: string[] = [], | ||
| extensions: readonly string[] = [], |
There was a problem hiding this comment.
Suggestion: The default empty extension list allows callers that omit extensions—notably the re-sign flow—to receive cached assets without validating or returning profiles for embedded extensions. Re-signing an IPA with an extension can therefore leave the extension using its old profile/signature while only the main app is updated. Pass the artifact app's extension bundle IDs from the caller or require them explicitly. [api mismatch]
Severity Level: Major ⚠️
- ❌ Apple re-signing can use incomplete extension signing assets.
- ⚠️ Embedded extensions may retain incompatible signing metadata.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/credentials/appleSigning.ts
**Line:** 88:88
**Comment:**
*Api Mismatch: The default empty extension list allows callers that omit `extensions`—notably the re-sign flow—to receive cached assets without validating or returning profiles for embedded extensions. Re-signing an IPA with an extension can therefore leave the extension using its old profile/signature while only the main app is updated. Pass the artifact app's extension bundle IDs from the caller or require them explicitly.
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
| extensions: readonly string[], | ||
| entitlements: Record<string, unknown> | undefined, | ||
| ): Effect.Effect<TargetSigningReadiness[], unknown> => { | ||
| const required = mapEntitlementsToCapabilities(entitlements).enable; |
There was a problem hiding this comment.
Suggestion: Extensions are always assigned an empty required-capability set, so preflight only detects whether an extension App ID is registered and never reports capabilities missing from that extension. An extension that requires, for example, push notifications or another entitlement not present on the main app will pass preflight and fail later during provisioning or export. Gather each target's entitlements and map its own required capabilities. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Extension capability gaps escape preflight warnings.
- ❌ Provisioning can fail later during archive or export.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/credentials/signingPreflight.ts
**Line:** 65:68
**Comment:**
*Incomplete Implementation: Extensions are always assigned an empty required-capability set, so preflight only detects whether an extension App ID is registered and never reports capabilities missing from that extension. An extension that requires, for example, push notifications or another entitlement not present on the main app will pass preflight and fail later during provisioning or export. Gather each target's entitlements and map its own required capabilities.
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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Superseded by land of tip stack #386 (same 12 domain commits). |
1 participant
Stack 7/12 of re-split HOLD #307
Domain:
credentialsBase:
refactor/types/readonly-stack-06-buildFull green tip:
refactor/foundation/readonly-types-fullLand stack in order. Intermediate PRs may not typecheck alone.
Summary by cubic
Refactor the credentials domain to adopt readonly types and safer immutability. No behavior changes; updates are type-focused to align with the readonly stack.
Schema.mutableusage; schemas now define immutable structures for accounts and apps arrays.readonlyarrays (e.g.,extensions,apps,accounts,containers,readiness).MutableDeepwhen constructing records (AccountRecord,SigningAssets,ApnsKeyRecord) to keep write paths mutable while exposing readonly types.apps,extensions).readonlyarrays.Written for commit f2f3c9c. Summary will update on new commits.