fix: Fix extractor.json being silently overwritten when it already exists but fails schema validation during auto-generation#813
Merged
kingston merged 1 commit intoMar 12, 2026
Conversation
…ists but fails schema validation during auto-generation
🦋 Changeset detectedLatest commit: 0805dac The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds a safeguard to prevent silent overwriting of extractor.json when it exists but fails schema validation. The system now throws an error requiring manual fixes before proceeding, along with test coverage and a changeset documenting the patch. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sync/src/templates/extractor/configs/try-create-extractor-json.ts (1)
73-87:⚠️ Potential issue | 🔴 CriticalMake the create step atomic to prevent data loss under concurrent execution.
The
existsSync()check on line 73 is racy withwriteFile()on lines 84–88: if multiple processes run concurrently, one process can createextractor.jsonafter another process checks for its existence but before that process writes, causing the file to be truncated. Use exclusive create mode (flag: 'wx') and translate theEEXISTerror to preserve the intended behavior without the race condition.Proposed fix
- if (fsAdapter.existsSync(extractorJsonPath)) { - throw new Error( - `extractor.json already exists at ${extractorJsonPath} but could not be loaded. Please fix the existing file before running extraction.`, - ); - } - // Create the extractor.json content const extractorConfig = { name: generatorPath, }; - await writeFile( - extractorJsonPath, - `${JSON.stringify(extractorConfig, null, 2)}\n`, - 'utf8', - ); + try { + await writeFile( + extractorJsonPath, + `${JSON.stringify(extractorConfig, null, 2)}\n`, + { encoding: 'utf8', flag: 'wx' }, + ); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'EEXIST') { + throw new Error( + `extractor.json already exists at ${extractorJsonPath} but could not be loaded. Please fix the existing file before running extraction.`, + ); + } + + throw error; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sync/src/templates/extractor/configs/try-create-extractor-json.ts` around lines 73 - 87, Replace the racy existsSync() + writeFile() flow by attempting an exclusive create when writing extractor.json: call writeFile(extractorJsonPath, JSON.stringify(extractorConfig,... ) + '\n', { encoding: 'utf8', flag: 'wx' }) and catch the EEXIST error to throw the same "extractor.json already exists..." Error as before (preserving extractorJsonPath in the message); keep the original extractorConfig generation and remove the prior existsSync() branch so concurrent processes cannot truncate the file.
🧹 Nitpick comments (1)
packages/sync/src/templates/extractor/configs/try-create-extractor-json.unit.test.ts (1)
96-120: Assert that the existing file was not modified.This test currently proves the helper rejects, but it does not prove the original
extractor.jsonsurvived untouched. Adding that post-condition would guard the exact regression this PR is fixing.Suggested test hardening
it('should throw error when extractor.json already exists', async () => { // Arrange const packagePath = '/test-package'; const generatorName = 'test-package#auth/login'; + const extractorJsonPath = + `${packagePath}/src/generators/auth/login/extractor.json`; const packageMap = new Map<string, string>([ ['test-package', '/test-package'], ]); vol.fromJSON({ [`${packagePath}/src/generators/auth/login/login.generator.ts`]: 'export const generator = {};', - [`${packagePath}/src/generators/auth/login/extractor.json`]: - '{"name": "INVALID NAME"}', + [extractorJsonPath]: '{"name": "INVALID NAME"}', }); // Act & Assert await expect( tryCreateExtractorJson({ packageMap, generatorName, }), ).rejects.toThrowError( /extractor\.json already exists at .+ but could not be loaded/, ); + expect(vol.toJSON()[extractorJsonPath]).toBe('{"name": "INVALID NAME"}'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sync/src/templates/extractor/configs/try-create-extractor-json.unit.test.ts` around lines 96 - 120, After the test asserts that tryCreateExtractorJson rejects for an existing extractor.json, also verify the file was not modified by reading the file contents (e.g., via fs.promises.readFile or vol.readFile) at `${packagePath}/src/generators/auth/login/extractor.json` and asserting it still equals the original string '{"name": "INVALID NAME"}'; update the 'should throw error when extractor.json already exists' test to perform this post-condition check using the same packagePath/generatorName/packageMap setup and the existing vol.fromJSON fixture so the test ensures the original extractor.json survived unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/sync/src/templates/extractor/configs/try-create-extractor-json.ts`:
- Around line 73-87: Replace the racy existsSync() + writeFile() flow by
attempting an exclusive create when writing extractor.json: call
writeFile(extractorJsonPath, JSON.stringify(extractorConfig,... ) + '\n', {
encoding: 'utf8', flag: 'wx' }) and catch the EEXIST error to throw the same
"extractor.json already exists..." Error as before (preserving extractorJsonPath
in the message); keep the original extractorConfig generation and remove the
prior existsSync() branch so concurrent processes cannot truncate the file.
---
Nitpick comments:
In
`@packages/sync/src/templates/extractor/configs/try-create-extractor-json.unit.test.ts`:
- Around line 96-120: After the test asserts that tryCreateExtractorJson rejects
for an existing extractor.json, also verify the file was not modified by reading
the file contents (e.g., via fs.promises.readFile or vol.readFile) at
`${packagePath}/src/generators/auth/login/extractor.json` and asserting it still
equals the original string '{"name": "INVALID NAME"}'; update the 'should throw
error when extractor.json already exists' test to perform this post-condition
check using the same packagePath/generatorName/packageMap setup and the existing
vol.fromJSON fixture so the test ensures the original extractor.json survived
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47cb8145-6495-4cb1-8b8f-83b68be23fa9
📒 Files selected for processing (3)
.changeset/fix-extractor-json-overwrite.mdpackages/sync/src/templates/extractor/configs/try-create-extractor-json.tspackages/sync/src/templates/extractor/configs/try-create-extractor-json.unit.test.ts
kingston
deleted the
kingston/eng-984-when-extracting-the-template-with-an-invalid-name-it-re
branch
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit