Skip to content
This repository was archived by the owner on Oct 31, 2025. It is now read-only.

Conversation

@Jazzcort
Copy link

@Jazzcort Jazzcort commented Aug 1, 2025

Description

This feature validates the config.yaml file only once during the activation phase. Default virtual references are treated as existing for both active and commented-out models. Any missing references will be automatically injected at the end of the models section. This behavior supports both flow-style and block-style YAML formats.

Screen recording or screenshot

Add missing $granite-code/models/chat

Screen.Recording.2025-08-01.at.3.49.41.PM.mov

Missing models section

Screen.Recording.2025-08-01.at.3.50.30.PM.mov

Empty models section

Screen.Recording.2025-08-01.at.4.50.17.PM.mov

Add missing references (flow-style)

Screen.Recording.2025-08-01.at.3.51.19.PM.mov
Screen.Recording.2025-08-01.at.4.19.35.PM.mov

Tests

Working on writing unit tests for this feature

@Jazzcort Jazzcort force-pushed the Jazzcort/add-virtual-reference branch 4 times, most recently from 8fe8b0e to 065e759 Compare August 4, 2025 17:28
@Jazzcort Jazzcort marked this pull request as ready for review August 4, 2025 17:45
@Jazzcort Jazzcort force-pushed the Jazzcort/add-virtual-reference branch from 065e759 to 62a57c7 Compare August 5, 2025 17:10
Copy link

@owtaylor owtaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good - various comments in detail

@owtaylor
Copy link

Also, note that this will make granite.writeContinueConfig not work correctly - I'm not sure that fixing it is that worthwhile since it will be non-trivial, but it does make our own lives easier if we want to work on Granite support in the upstream. Maybe make that file an "Not implemented" toast, and file an issue in granite-code/granite-code to fix it. (The toast can even reference the issue.)

Copy link
Author

@Jazzcort Jazzcort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! Working on it!

@Jazzcort Jazzcort force-pushed the Jazzcort/add-virtual-reference branch 2 times, most recently from 8913778 to cd063ab Compare August 12, 2025 19:42
@Jazzcort
Copy link
Author

@owtaylor Let me know if the test cases looks good to you! 😁

@Jazzcort Jazzcort force-pushed the Jazzcort/add-virtual-reference branch 4 times, most recently from 558dd8b to 624588c Compare August 20, 2025 15:53
@Jazzcort
Copy link
Author

@fbricon I made those I/O functions asynchronous, but because the constructor function is not an asynchronous function, we can not do await validateVirtualReferences(ide). Therefore, I did void validateVirtualReferences(ide) instead.

This feature validates the config.yaml file only once during the activation phase. Default
virtual references are treated as existing for both active and commented-out models. Any
missing references will be automatically injected at the end of the models section. This
behavior supports both flow-style and block-style YAML formats.
@fbricon fbricon force-pushed the Jazzcort/add-virtual-reference branch from 624588c to a2c8e24 Compare August 21, 2025 14:51
@fbricon fbricon merged commit 714f35c into granite/customization Aug 21, 2025
10 checks passed
@fbricon
Copy link

fbricon commented Aug 21, 2025

@Jazzcort Thanks! I didn't know about the async => void hack. As long as it's consistent with the existing code, it's fine by me.
I just renamed testFilePath to configFilePath in the validateVirtualReferences signature, and I merged it.

@Jazzcort
Copy link
Author

Thanks for the review and suggestion! @fbricon
I didn't know fs.readFileSync can pause the entire event loop for nodejs until it finishes. 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants