refactor(providers): readonly-domain-types (stack 4/12, re-split #307) - #377
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:XS
PR Summary by QodoRefactor providers for readonly domain types (credentials tests + local storage)
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const artifactIndex = yield* readIndex(); | ||
| artifactIndex.unshift({ ...artifact, path: destination }); | ||
| yield* writeIndex(artifactIndex); | ||
| yield* writeIndex([{ ...artifact, path: destination }, ...artifactIndex]); |
There was a problem hiding this comment.
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.
(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
|
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 |
Code Review by Qodo
1. Readonly index type mismatch
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const writeIndex = (artifactIndex: readonly BuildArtifact[]) => | ||
| artifactRetention.writeIndex(artifactIndex, artifactIndexPath); |
There was a problem hiding this comment.
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
![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 2 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/providers/storage/local.ts">
<violation number="1" location="src/providers/storage/local.ts:40">
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.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const artifactIndex = yield* readIndex(); | ||
| artifactIndex.unshift({ ...artifact, path: destination }); | ||
| yield* writeIndex(artifactIndex); | ||
| yield* writeIndex([{ ...artifact, path: destination }, ...artifactIndex]); |
There was a problem hiding this comment.
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>
|
Superseded by land of tip stack #386 (same 12 domain commits). |
1 participant
Stack 4/12 of re-split HOLD #307
Domain:
providersBase:
refactor/types/readonly-stack-03-googleFull green tip:
refactor/foundation/readonly-types-fullLand stack in order. Intermediate PRs may not typecheck alone.
Summary by cubic
Make providers compatible with readonly types by removing mutation in local storage and using a mutable test context when needed. Improves type safety and immutability without changing behavior.
src/providers/storage/local.ts:writeIndexnow acceptsreadonly BuildArtifact[]; replaced in-placeunshiftwith[newItem, ...existing].src/providers/credentials/local.test.ts: useMutableDeep<ResolvedBuildContext>from@core/types/mutable.jsfor test-only mutations.Written for commit 5c6fdbc. Summary will update on new commits.