Skip to content

refactor(config): readonly-domain-types (stack 8/12, re-split #307) #381

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(config): readonly-domain-types (stack 8/12, re-split #307) #381

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
3 changes: 2 additions & 1 deletion src/core/config/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
DEFAULT_SUBMITTER,
} from '../types/config.js';
import { LaunchPaths, type LaunchPathsService } from '../services/paths.js';
import type { MutableDeep } from '../types/mutable.js';
/**
* Absolute path to THIS package's own public entry (`defineConfig` + the config types), resolved
* relative to the loader so it points at whichever copy is actually running - the globally-installed
Expand Down Expand Up @@ -205,7 +206,7 @@ const appDescriptorFromConfig = (
appHandle = expoConfig['name'];
}
if (appHandle === undefined) return null;
const descriptor: AppDescriptor = {
const descriptor: MutableDeep<AppDescriptor> = {
name: appHandle.toLowerCase(),
dir: appDirectory,
configPath,
Expand Down
8 changes: 4 additions & 4 deletions src/core/config/env.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export const loadDotenvFile = (filePath: string) =>
* snag a publishable `EXPO_PUBLIC_..._KEY`. The single matching rule shared by {@link resolveEnv} (drops
* matches before injection) and {@link missingKeys} (exempts matches from the gate), so the two agree.
*/
export const isEnvExcluded = (name: string, patterns: string[]): boolean => {
export const isEnvExcluded = (name: string, patterns: readonly string[]): boolean => {
for (const pattern of patterns) {
if (pattern.endsWith('*')) {
if (name.startsWith(pattern.slice(0, -1))) return true;
Comment on lines 47 to 49

@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: A configured pattern of "*" is accepted by the schema, but this treats it as a prefix wildcard with an empty prefix, so every environment variable matches and is removed from every layer. This can silently produce an empty build environment and exempt every documented key from missing-key validation. Reject "*" or handle it explicitly according to the intended configuration semantics. [incorrect condition logic]

Severity Level: Major ⚠️ 
- ❌ Build environment can be emptied by one accepted configuration entry.
- ⚠️ Missing-key validation is bypassed for every documented variable.
- ❌ Builds requiring environment values can fail later downstream.

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/config/env.ts
**Line:** 47:49
**Comment:**
	*Incorrect Condition Logic: A configured pattern of `"*"` is accepted by the schema, but this treats it as a prefix wildcard with an empty prefix, so every environment variable matches and is removed from every layer. This can silently produce an empty build environment and exempt every documented key from missing-key validation. Reject `"*"` or handle it explicitly according to the intended configuration semantics.

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 @@ -64,7 +64,7 @@ export const isEnvExcluded = (name: string, patterns: string[]): boolean => {
export const missingKeys = (
appDirectory: string,
environment: Record<string, string>,
excludedPatterns: string[] = [],
excludedPatterns: readonly string[] = [],
) =>
Effect.gen(function* () {
const pathService = yield* Path.Path;
Expand Down Expand Up @@ -138,7 +138,7 @@ export type ResolveEnvInput = {
secrets?: Record<string, string> | undefined;
cliEnv?: Record<string, string> | undefined;
includeLocal?: boolean | undefined;
envExclude?: string[] | undefined;
envExclude?: readonly string[] | undefined;
};
/**
* Resolve env through the single precedence ladder (lowest -> highest, later overrides earlier):
Expand Down Expand Up @@ -185,7 +185,7 @@ export const resolveEnv = (input: ResolveEnvInput) =>
// Hard denylist: an excluded name is skipped in EVERY layer, so it can never land in the result no
// matter which layer (incl. the final `--env`) set it - exclusion wins over precedence by design. Names
// some layer actually tried to set are recorded, so the build log reports real drops, not the raw list.
let excludedPatterns: string[] = [];
let excludedPatterns: readonly string[] = [];
if (input.envExclude !== undefined) excludedPatterns = input.envExclude;
const excludedSeen = new Set<string>();
const values: Record<string, string> = {};
Expand Down
6 changes: 3 additions & 3 deletions src/core/config/toolchain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ export const remoteToolchainPreflight = (mode: 'install' | 'assert'): string =>
*/
export type ToolchainIo = {
exists(command: string): Effect.Effect<boolean, unknown>;
run(command: string, args: string[]): Effect.Effect<void, unknown>;
run(command: string, args: readonly string[]): Effect.Effect<void, unknown>;
confirm(message: string): Effect.Effect<boolean, unknown>;
Comment on lines 198 to 201

@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 newly exposed run operation propagates non-zero command failures from executeCommand directly to callers. In particular, ensureCcacheInstalled documents that failed installation/configuration must never throw and should return a skipped result, but its brew install and configuration calls now fail the whole effect instead; the same failure propagation can abort ensureToolchain instead of returning its documented boolean outcome. Map command failures to the appropriate best-effort result or log and continue where the contract requires it. [api mismatch]

Severity Level: Major ⚠️ 
- ❌ Accepted ccache install failures can abort iOS builds.
- ❌ `launch doctor --fix` can fail instead of reporting readiness.
- ⚠️ Optional caching becomes a hard build dependency after acceptance.

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/config/toolchain.ts
**Line:** 198:201
**Comment:**
	*Api Mismatch: The newly exposed `run` operation propagates non-zero command failures from `executeCommand` directly to callers. In particular, `ensureCcacheInstalled` documents that failed installation/configuration must never throw and should return a skipped result, but its `brew install` and configuration calls now fail the whole effect instead; the same failure propagation can abort `ensureToolchain` instead of returning its documented boolean outcome. Map command failures to the appropriate best-effort result or log and continue where the contract requires it.

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

confirmText(message: string, expected: string): Effect.Effect<boolean, unknown>;
log(message: string): Effect.Effect<void, unknown>;
Expand Down Expand Up @@ -226,7 +226,7 @@ export type EnsureToolchainOptions = {
/** Return the tools from `tools` whose command isn't currently on `PATH`. */
const detectMissing = (
io: Pick<ToolchainIo, 'exists'>,
tools: Tool[],
tools: readonly Tool[],
): Effect.Effect<Tool[], unknown> =>
Effect.filter(tools, (tool) => io.exists(tool.command).pipe(Effect.map((exists) => !exists)), {
concurrency: 1,
Expand Down Expand Up @@ -343,7 +343,7 @@ export const ensureCcacheInstalled = (options: {
*/
const installBrewTools = (
io: ToolchainIo,
brewTools: Tool[],
brewTools: readonly Tool[],
assumeYes: boolean,
): Effect.Effect<void, unknown> =>
Effect.gen(function* () {
Expand Down
Loading