Skip to content

refactor(providers): readonly-domain-types (stack 4/12, re-split #307) #377

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(providers): readonly-domain-types (stack 4/12, re-split #307) #377

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/providers/credentials/local.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type { ResolvedBuildContext } from '@core/types/config.js';
import type { AscKey } from '@core/types/credentials.js';
import type { CredentialsProvider } from '@core/types/providers.js';
import { makeLocalCredentialsProvider } from './local.js';
import type { MutableDeep } from '@core/types/mutable.js';

type LocalCredentialsTestState = {
appleKeys: Map<string, AscKey>;
Expand Down Expand Up @@ -55,7 +56,7 @@ const runWithLocalCredentials = <Success, Failure>(

/** Build context containing only the fields the local credentials provider reads. */
const iosContext = (account?: string): ResolvedBuildContext => {
const buildContext: ResolvedBuildContext = {
const buildContext: MutableDeep<ResolvedBuildContext> = {
platform: 'ios',
app: {
name: 'sampleapp',
Expand Down
5 changes: 2 additions & 3 deletions src/providers/storage/local.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export const makeLocalStorageProvider = (directoryOverride?: string) =>
const objectsDirectory = pathService.join(baseDirectory, 'objects');
const artifactIndexPath = pathService.join(baseDirectory, 'index.json');
const readIndex = () => artifactRetention.readIndex(artifactIndexPath);
const writeIndex = (artifactIndex: BuildArtifact[]) =>
const writeIndex = (artifactIndex: readonly BuildArtifact[]) =>
artifactRetention.writeIndex(artifactIndex, artifactIndexPath);
Comment on lines +26 to 27

@qodo-code-review qodo-code-review 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.

Action required

1. Readonly index type mismatch 🐞 Bug ≡ Correctness

makeLocalStorageProvider now defines writeIndex to accept readonly BuildArtifact[] but passes
that value to ArtifactRetention.writeIndex, whose service contract currently requires
BuildArtifact[] (mutable). This causes a TypeScript incompatibility (readonly array not assignable
to mutable array) and can break typechecking/builds for the local storage provider.
Agent Prompt 
## Issue description
`src/providers/storage/local.ts` changed `writeIndex` to accept `readonly BuildArtifact[]`, but it forwards that parameter to `artifactRetention.writeIndex`, which is typed to require a mutable `BuildArtifact[]`. TypeScript will reject passing a readonly array to a function that may mutate it.

## Issue Context
The underlying implementation (`writeArtifactIndex`) simply serializes the array and does not mutate it, so the easiest fix is to make the `ArtifactRetention` write API accept `readonly BuildArtifact[]` (or alternatively, keep `writeIndex` mutable in the provider).

## Fix Focus Areas
- src/providers/storage/local.ts[25-41]
- src/core/services/artifactRetention.ts[11-21]
- src/core/build/artifactRetention.ts[45-56]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

const objectPath = (objectKey: string): string =>
pathService.join(objectsDirectory, ...objectKey.split('/'));
Expand All @@ -37,8 +37,7 @@ export const makeLocalStorageProvider = (directoryOverride?: string) =>
const destination = pathService.join(baseDirectory, artifactId);
yield* fileSystem.copy(artifact.path, destination);
const artifactIndex = yield* readIndex();
artifactIndex.unshift({ ...artifact, path: destination });
yield* writeIndex(artifactIndex);
yield* writeIndex([{ ...artifact, path: destination }, ...artifactIndex]);

@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: If writeIndex fails after the copy succeeds, put returns a failure while leaving the copied binary at destination without an index entry. Retries or repeated failures can accumulate unreachable artifact files; remove the copied file when index persistence fails or make the operation recoverable. [resource leak]

Severity Level: Major ⚠️ 
- ❌ Failed stores leave unindexed artifact binaries.
- ⚠️ Repeated failures consume artifact-directory disk space.
- ⚠️ `list()` cannot discover orphaned files.

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/providers/storage/local.ts
**Line:** 40:40
**Comment:**
	*Resource Leak: If `writeIndex` fails after the copy succeeds, `put` returns a failure while leaving the copied binary at `destination` without an index entry. Retries or repeated failures can accumulate unreachable artifact files; remove the copied file when index persistence fails or make the operation recoverable.

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

@cubic-dev-ai cubic-dev-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.

P2: If writeIndex fails after the file copy has already succeeded, the copied binary at destination is left on disk without a corresponding index entry. Repeated failures can accumulate orphaned artifact files that list() can never discover. Consider cleaning up the copied file (or making the write recoverable) when index persistence fails.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/providers/storage/local.ts, line 40:

<comment>If `writeIndex` fails after the file copy has already succeeded, the copied binary at `destination` is left on disk without a corresponding index entry. Repeated failures can accumulate orphaned artifact files that `list()` can never discover. Consider cleaning up the copied file (or making the write recoverable) when index persistence fails.</comment>

<file context>
@@ -37,8 +37,7 @@ export const makeLocalStorageProvider = (directoryOverride?: string) =>
           const artifactIndex = yield* readIndex();
-          artifactIndex.unshift({ ...artifact, path: destination });
-          yield* writeIndex(artifactIndex);
+          yield* writeIndex([{ ...artifact, path: destination }, ...artifactIndex]);
           return { id: artifactId, location: destination };
         }),
</file context>

return { id: artifactId, location: destination };
}),
list: readIndex,
Expand Down
Loading