refactor(config): readonly-domain-types (stack 8/12, re-split #307) - #381
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 · |
|
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 |
PR Summary by QodoRefactor config domain consumers for readonly types
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
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. |
| for (const pattern of patterns) { | ||
| if (pattern.endsWith('*')) { | ||
| if (name.startsWith(pattern.slice(0, -1))) return true; |
There was a problem hiding this comment.
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.(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| 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>; |
There was a problem hiding this comment.
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.(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|
Superseded by land of tip stack #386 (same 12 domain commits). |
Stack 8/12 of re-split HOLD #307
Domain:
configBase:
refactor/types/readonly-stack-07-credentialsFull green tip:
refactor/foundation/readonly-types-fullLand stack in order. Intermediate PRs may not typecheck alone.
Summary by cubic
Refactored the config domain to use readonly types for env patterns, args, and tool lists to prevent accidental mutation. Also typed the built app descriptor as
MutableDeep<AppDescriptor>where internal mutation is needed.env.ts: Marked exclude patterns and related params asreadonly string[]; updatedResolveEnvInput.envExclude.toolchain.ts: UpdatedToolchainIo.runtoargs: readonly string[];detectMissingandinstallBrewToolsnow takereadonly Tool[].config.ts: ImportedMutableDeepand constructed the descriptor asMutableDeep<AppDescriptor>.Written for commit 014e7d0. Summary will update on new commits.