fix(cli): stop leaking a file handle in create-settings-file - #365
Open
Eljees wants to merge 1 commit into
Open
Conversation
The existence check opened the settings file and never closed it (CodeQL: file is opened but not closed). Use os.path.exists instead and keep a single write path. Signed-off-by: Eljees <[email protected]>
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
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.
Fixes #226
create_settingsusedopen()as an existence check and never closed the handle — that is the CodeQLfinding at
cli.py:177:Replaced with an explicit
os.path.exists()check, so no handle is opened for the check at all, and thewrite path exists once instead of twice. Behaviour is unchanged: without
--forcean existing file is keptand the warning is logged; with
--force, or when the file does not exist, the default settings are written.The redundant
settings_file.close()calls inside thewithblocks went away with the duplication.Test
tests/test_cli.py(new): invoking the command against an existing file without--forceleaves the fileuntouched — this is the branch the fix rewrote.
One thing I could not cover, and why
I wanted to also assert that the file is written when it does not exist, but that path is currently broken
on
main, independently of this PR:DEFAULT_SETTINGS["middlewares"]["modelscan.middlewares.FormatViaExtensionMiddleware"]["formats"]is keyed bySupportedModelFormats.*, which arePropertyinstances, andtomlkit.dumps()only accepts string keys — somodelscan create-settings-filefails for everyone right now (tomlkit 0.13, within the>=0.12.3,<0.14.0pin).That is a separate bug from this one; happy to open an issue for it, or send a fix if you tell me which shape
you prefer — string keys in
DEFAULT_SETTINGS, or a conversion at serialisation time so the in-memory lookupskeep working.
AI-assisted (LLM used for drafting); the change, the test run and the reproduction above are mine.