refactor(services): readonly-domain-types (stack 11/12, re-split #307) - #384
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:S
PR Summary by Qodorefactor(services): use readonly array types in service interfaces
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| * sometimes precedes the trailing lines. | ||
| */ | ||
| const reportFailure = (label: string, tail: string[], logFile: string) => | ||
| const reportFailure = (label: string, tail: readonly string[], logFile: string) => |
There was a problem hiding this comment.
Suggestion: The failure tail is populated from the raw child-process output, while build logs are only redacted when persisted. Reporting tail directly can therefore print API keys, bearer tokens, JWTs, or other secret-looking values to the terminal whenever a build fails. Redact each tail line before adding it to the failure report, especially when buildLog is active. [security]
Severity Level: Critical 🚨
- ❌ Interactive failed builds can print raw credentials.
- ❌ Fastlane and Gradle build diagnostics may expose secrets.
- ⚠️ Persisted logs are redacted but terminal output is not.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/services/progress.ts
**Line:** 138:138
**Comment:**
*Security: The failure tail is populated from the raw child-process output, while build logs are only redacted when persisted. Reporting `tail` directly can therefore print API keys, bearer tokens, JWTs, or other secret-looking values to the terminal whenever a build fails. Redact each tail line before adding it to the failure report, especially when `buildLog` is active.
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
| readonly readIndex: (indexPath: string) => Effect.Effect<readonly BuildArtifact[]>; | ||
| readonly writeIndex: ( | ||
| artifactIndex: BuildArtifact[], | ||
| artifactIndex: readonly BuildArtifact[], | ||
| indexPath: string, | ||
| ) => Effect.Effect<void, PlatformError>; | ||
| readonly prune: ( |
There was a problem hiding this comment.
Suggestion: The separate readIndex and writeIndex operations allow local storage uploads to perform an unsynchronized read-modify-write. Concurrent put effects can both read the same index and then overwrite each other, losing one artifact entry. Serialize index updates or provide an atomic append/update operation. [race condition]
Severity Level: Major ⚠️
- ⚠️ Concurrent local builds can lose history entries.
- ❌ `builds history` omits successfully stored artifacts.
- ⚠️ Retention decisions use incomplete artifact indexes.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/services/artifactRetention.ts
**Line:** 13:18
**Comment:**
*Race Condition: The separate `readIndex` and `writeIndex` operations allow local storage uploads to perform an unsynchronized read-modify-write. Concurrent `put` effects can both read the same index and then overwrite each other, losing one artifact entry. Serialize index updates or provide an atomic append/update operation.
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 cache lookup now accepts extension bundle IDs, but the local credentials provider supplies only the statically configured app.iosExtensions. The build pipeline separately discovers extension targets from the generated Xcode project, so a project with an unconfigured/discovered extension can reuse signing assets without an extensionProfiles entry for that target; the generated export options then omits the extension profile and Xcode export fails. Pass the resolved extension set into credential resolution or make cache reuse validate all discovered targets before returning assets. [api mismatch]
Severity Level: Major ⚠️
- ❌ Local iOS builds can fail during signing export.
- ❌ Discovered extensions lack export profile mappings.
- ⚠️ Bare projects bypass static extension configuration.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/services/localCredentialsStore.ts
**Line:** 38:38
**Comment:**
*Api Mismatch: The cache lookup now accepts extension bundle IDs, but the local credentials provider supplies only the statically configured `app.iosExtensions`. The build pipeline separately discovers extension targets from the generated Xcode project, so a project with an unconfigured/discovered extension can reuse signing assets without an `extensionProfiles` entry for that target; the generated export options then omits the extension profile and Xcode export fails. Pass the resolved extension set into credential resolution or make cache reuse validate all discovered targets before returning assets.
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}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/core/services/progress.ts">
<violation number="1" location="src/core/services/progress.ts:138">
P1: reportFailure prints the raw child-process tail directly to the terminal on build failure. Since build logs are only redacted when persisted to disk, any secret-looking values (API keys, bearer tokens, JWTs) present in the last lines of output will be echoed unredacted to the console. Consider redacting tail lines the same way persisted logs are redacted before including them in the failure report.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| * sometimes precedes the trailing lines. | ||
| */ | ||
| const reportFailure = (label: string, tail: string[], logFile: string) => | ||
| const reportFailure = (label: string, tail: readonly string[], logFile: string) => |
There was a problem hiding this comment.
P1: reportFailure prints the raw child-process tail directly to the terminal on build failure. Since build logs are only redacted when persisted to disk, any secret-looking values (API keys, bearer tokens, JWTs) present in the last lines of output will be echoed unredacted to the console. Consider redacting tail lines the same way persisted logs are redacted before including them in the failure report.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/core/services/progress.ts, line 138:
<comment>reportFailure prints the raw child-process tail directly to the terminal on build failure. Since build logs are only redacted when persisted to disk, any secret-looking values (API keys, bearer tokens, JWTs) present in the last lines of output will be echoed unredacted to the console. Consider redacting tail lines the same way persisted logs are redacted before including them in the failure report.</comment>
<file context>
@@ -135,7 +135,7 @@ const logStamp = (epochMilliseconds: number): string => {
* sometimes precedes the trailing lines.
*/
-const reportFailure = (label: string, tail: string[], logFile: string) =>
+const reportFailure = (label: string, tail: readonly string[], logFile: string) =>
Effect.gen(function* () {
const lines = [
</file context>
|
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 |
|
Superseded by land of tip stack #386 (same 12 domain commits). |
1 participant
Stack 11/12 of re-split HOLD #307
Domain:
servicesBase:
refactor/types/readonly-stack-10-readinessFull green tip:
refactor/foundation/readonly-types-fullLand stack in order. Intermediate PRs may not typecheck alone.
Summary by cubic
Make array parameters and return types in core service interfaces readonly to prevent accidental mutation and align with the readonly-domain-types refactor. No runtime behavior changes.
deviceIdsis nowreadonly string[]increateAdhocProfile.readIndexandwriteIndexusereadonly BuildArtifact[].extensions?isreadonly string[].boxandshippedacceptreadonly string[]receipt lines.reportFailuretail andrunWithProgressargs arereadonly string[].clearSandboxTesterPurchaseHistorytakesreadonly string[].rsyncUpexcludesisreadonly string[].Written for commit fad98ea. Summary will update on new commits.