Skip to content

refactor(credentials): readonly-domain-types (stack 7/12, re-split #307) #380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
YosefHayim wants to merge 1 commit into from
Closed

refactor(credentials): readonly-domain-types (stack 7/12, re-split #307) #380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 22 additions & 25 deletions src/core/credentials/accounts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { deleteSecret, getSecret, setSecret } from './keychain.js';
import { migrateLegacySigningIndex, p12PasswordAccount } from './appleSigning.js';
import { AppStoreIdentityService } from '../services/appStoreIdentity.js';
import type { LaunchSecretStoreService } from '../services/secretStore.js';
import type { MutableDeep } from '../types/mutable.js';
/** Secret-store account holding one Apple account's `.p8` PEM, namespaced by Key ID. */
const p8Account = (keyId: string): string => {
return `asc-p8:${keyId}`;
Expand All @@ -25,23 +26,19 @@ export type AccountFailure = Readonly<{
readonly message: string;
}>;
export const makeAccountFailure = Data.tagged<AccountFailure>('AccountFailure');
const AccountRecordSchema: Schema.Schema<AccountRecord> = Schema.mutable(
Schema.Struct({
keyId: Schema.String,
issuerId: Schema.String,
label: Schema.String,
teamId: Schema.optionalWith(Schema.String, { exact: true }),
apps: Schema.optionalWith(Schema.mutable(Schema.Array(Schema.String)), { exact: true }),
addedAt: Schema.String,
resolvedAt: Schema.optionalWith(Schema.String, { exact: true }),
}),
);
const AccountsFileSchema: Schema.Schema<AccountsFile> = Schema.mutable(
Schema.Struct({
active: Schema.NullOr(Schema.String),
accounts: Schema.mutable(Schema.Array(AccountRecordSchema)),
}),
);
const AccountRecordSchema: Schema.Schema<AccountRecord> = Schema.Struct({
keyId: Schema.String,
issuerId: Schema.String,
label: Schema.String,
teamId: Schema.optionalWith(Schema.String, { exact: true }),
apps: Schema.optionalWith(Schema.Array(Schema.String), { exact: true }),
addedAt: Schema.String,
resolvedAt: Schema.optionalWith(Schema.String, { exact: true }),
});
const AccountsFileSchema: Schema.Schema<AccountsFile> = Schema.Struct({
active: Schema.NullOr(Schema.String),
accounts: Schema.Array(AccountRecordSchema),
});
type AccountStorageRequirements = FileSystem.FileSystem | LaunchPathsService | Path.Path;
const emptyAccountsFile = (): AccountsFile => ({ active: null, accounts: [] });
/** ISO-8601 stamp for `addedAt`/`resolvedAt`. */
Expand Down Expand Up @@ -139,7 +136,7 @@ export const findAccount = (
);
/** Match an account by its label or Key ID, case-insensitively - the selector form users type. */
export const matchAccount = (
accounts: AccountRecord[],
accounts: readonly AccountRecord[],
selector: string,
): AccountRecord | undefined => {
const needle = selector.trim().toLowerCase();
Expand Down Expand Up @@ -209,14 +206,14 @@ export const addAccount = (
if (!hasIdentity && input.apps !== undefined) hasIdentity = input.apps.length > 0;
let addedAt = existing?.addedAt;
if (addedAt === undefined) addedAt = timestamp;
const record: AccountRecord = {
const record: MutableDeep<AccountRecord> = {
keyId: input.keyId,
issuerId: input.issuerId,
label: input.label,
addedAt,
};
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];

@codeant-ai codeant-ai Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in VSCode Claude

(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
👍 | 👎

if (hasIdentity) record.resolvedAt = timestamp;
let accounts = [...file.accounts, record];
if (existing) {
Expand All @@ -232,16 +229,16 @@ export const addAccount = (
export const updateAccountIdentity = (
keyId: string,
teamId: string | null,
apps: string[],
apps: readonly string[],
): Effect.Effect<void, AccountFailure, AccountStorageRequirements> =>
Effect.gen(function* () {
const file = yield* readAccounts();
const timestamp = yield* currentTimestamp();
const accounts = file.accounts.map((account) => {
if (account.keyId !== keyId) return account;
const next: AccountRecord = { ...account, resolvedAt: timestamp };
if (teamId != null) next.teamId = teamId;
if (apps.length > 0) next.apps = apps;
let next: AccountRecord = { ...account, resolvedAt: timestamp };
if (teamId != null) next = { ...next, teamId };
if (apps.length > 0) next = { ...next, apps: [...apps] };
Comment on lines +239 to +241

@codeant-ai codeant-ai Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in VSCode Claude

(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
👍 | 👎

return next;
});
yield* writeAccounts({ active: file.active, accounts });
Expand Down Expand Up @@ -364,7 +361,7 @@ export const decideBuildAccount = (file: AccountsFile, selector?: string): Build
export type ResolveBuildAccountOptions = {
selector?: string | undefined;
interactive: boolean;
pick: (accounts: AccountRecord[]) => Effect.Effect<AccountRecord, unknown>;
pick: (accounts: readonly AccountRecord[]) => Effect.Effect<AccountRecord, unknown>;
};
/**
* Resolve the account a build should use, applying {@link decideBuildAccount} and then either using
Expand Down
4 changes: 2 additions & 2 deletions src/core/credentials/appleSigning.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ const seedCredentials = (
uuid: string;
}
>,
installedUuids: string[],
installedUuids: readonly string[],
): void => {
const dir = join(home.dir, '.launch', 'credentials', KEY_ID);
mkdirSync(dir, { recursive: true });
Expand Down Expand Up @@ -182,7 +182,7 @@ describe('profileStaleAgainstCapabilities - regenerate-vs-reuse decision (#261)'
profileContent: 'base64-bytes',
};
/** A client stub exposing only the one read this decision makes - no network. */
function clientWithCapabilities(types: string[]) {
function clientWithCapabilities(types: readonly string[]) {
return {
listBundleIdCapabilities: vi.fn(() =>
Effect.succeed(
Expand Down
5 changes: 3 additions & 2 deletions src/core/credentials/appleSigning.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { FileSystem, Path } from '@effect/platform';
import { Effect, Schema } from 'effect';
import type { Platform } from '../types/app.js';
import type { AscKey, SigningAssets } from '../types/credentials.js';
import type { MutableDeep } from '../types/mutable.js';
import type { Logger } from '../services/logger.js';
import { adHocProfileType, appStoreProfileType, platformLabel } from '../services/platform.js';
import {
Expand Down Expand Up @@ -84,7 +85,7 @@ export const describeStoredCredentials = (
export const loadCachedSigningAssets = (
keyId: string,
bundleId: string,
extensions: string[] = [],
extensions: readonly string[] = [],

@codeant-ai codeant-ai Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in VSCode Claude

(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
👍 | 👎

): Effect.Effect<
SigningAssets | null,
never,
Expand Down Expand Up @@ -121,7 +122,7 @@ export const loadCachedSigningAssets = (
return null;
extensionProfiles[ext] = extProfile.name;
}
const signingAssets: SigningAssets = {
const signingAssets: MutableDeep<SigningAssets> = {
bundleId,
teamId: profile.teamId,
certName: DISTRIBUTION_CERT_NAME,
Expand Down
2 changes: 1 addition & 1 deletion src/core/credentials/capabilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ export const APP_GROUP_PORTAL_URL =
* to-do: which groups, where to create them, and which bundle ids must join. Pure - for the build path to
* warn before archiving and for unit tests.
*/
export const appGroupPortalNotice = (containers: string[]): string | null => {
export const appGroupPortalNotice = (containers: readonly string[]): string | null => {
if (containers.length === 0) return null;
const groups = containers.map((id) => `"${id}"`).join(', ');
let plural = 'App Groups';
Expand Down
3 changes: 2 additions & 1 deletion src/core/credentials/pushKeyStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
import { getSecret, setSecret } from './keychain.js';
import { decodeP8, encodeP8 } from './accounts.js';
import type { LaunchSecretStoreService } from '../services/secretStore.js';
import type { MutableDeep } from '../types/mutable.js';
/** Secret-store account holding one APNs key's `.p8` PEM, namespaced by Key ID. */
const apnsAccount = (keyId: string): string => {
return `apns-p8:${keyId}`;
Expand Down Expand Up @@ -93,7 +94,7 @@ export const importPushKey = (
const timestamp = yield* currentTimestamp();
let importedAt = existing?.importedAt;
if (importedAt === undefined) importedAt = timestamp;
const record: ApnsKeyRecord = {
const record: MutableDeep<ApnsKeyRecord> = {
keyId: input.keyId,
importedAt,
};
Expand Down
8 changes: 5 additions & 3 deletions src/core/credentials/signingPreflight.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export const resolveExtensionBundleIdsForApp = (
): Effect.Effect<string[], unknown, FileSystem.FileSystem | Path.Path> =>
Effect.gen(function* () {
const pathService = yield* Path.Path;
let configured: string[] = [];
let configured: readonly string[] = [];
if (app.iosExtensions !== undefined) configured = app.iosExtensions;
const nativeDirectory = pathService.join(app.dir, 'ios');
const discovered = yield* discoverExtensionBundleIds(nativeDirectory, app.bundleId);
Expand All @@ -59,7 +59,7 @@ export const appGroupPreflightNotice = (
export const gatherTargetSigningReadiness = (
asc: SigningPreflightAscApi,
bundleId: string,
extensions: string[],
extensions: readonly string[],
entitlements: Record<string, unknown> | undefined,
): Effect.Effect<TargetSigningReadiness[], unknown> => {
const required = mapEntitlementsToCapabilities(entitlements).enable;

@codeant-ai codeant-ai Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in VSCode Claude

(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
👍 | 👎

Expand All @@ -85,7 +85,9 @@ export const gatherTargetSigningReadiness = (
);
};
/** Turn readiness facts into build-time warning strings (best-effort - never throws). */
export const signingPreflightWarnings = (readiness: TargetSigningReadiness[]): string[] => {
export const signingPreflightWarnings = (
readiness: readonly TargetSigningReadiness[],
): string[] => {
return multiTargetSigningWarnings(readiness);
};
/** Turn readiness facts into doctor checks - unregistered/missing-capability targets fail the run. */
Expand Down
Loading