opencode: Validate config files using existing schemas - #9692
Conversation
db7c5ba to
96ced5c
Compare
khaneliman
left a comment
There was a problem hiding this comment.
I'm not comfortable merging this in its current form. Beyond the inline activation-ordering issue, resolving mutable remote $schema URLs during activation makes a switch depend on network availability and upstream schema changes. Existing tests do not execute the onChange hook, the repository's own OpenCode configuration fixture fails this validator invocation despite the targeted tests passing.
I'd prefer pinned-schema validation of the generated store artifacts before writeBoundary (ideally at build time), with tests covering valid, invalid, and offline behavior.
|
The schemas are available in |
96ced5c to
13e82cd
Compare
| let | ||
| jsonschemaCli = lib.getExe pkgs.jsonschema-cli; | ||
| in | ||
| if schema == null then |
There was a problem hiding this comment.
This is a workaround because the theme schema is not exposed by opencode.passthru.jsonschema. For those cases, jsonschema-cli validate performs validations using the $schema metadata from the JSON file.
❯ jsonschema-cli validate ~/.config/opencode/opencode.json
Schema is valid
There was a problem hiding this comment.
Excelent! I'll re-add theme validations then.
|
I addressed the concerns. I'll look into the failing checks later this week. I tested locally, and all |
176ef97 to
54603d3
Compare
jsonschema-cli54603d3 to
257eda2
Compare
|
The tests pass locally and on the macos-latest and ubuntu-latest runners; however, these two fail with the following errors: aarch64-darwin.test-chunk-15 I need to look into this further. |
bb87ef5 to
51d601e
Compare
|
@khaneliman, after some back and forth, I figured out the problem with the tests was caused by |
delafthi
left a comment
There was a problem hiding this comment.
Sry, for the late review. I really like these changes. Most of the comments are just suggestions.
| ${lib.getExe pkgs.check-jsonschema} \ | ||
| --schemafile ${lib.escapeShellArg schema} \ | ||
| ${lib.escapeShellArgs instances} >&2 | ||
| ''; |
There was a problem hiding this comment.
What's the incentive behind this option? Is the validator really necessary for this use case?
If we need an escape hatch, I'd prefer and option like:
validate = {
config = <true|false>;
tui = <true|false>;
themes = <true|false>;
};More broadly, JSON schema validation isn't something unique and other modules could benefit from such a helper that accepts a schema, a list of files to validate and optionally extra arguments. @khaneliman What do you think?
There was a problem hiding this comment.
@delafthi I added this option to allow the validator to be customized, as I found a few CLI tools with similar capabilities. That is why in the example field the jsonschema-cli CLI tool is mentioned as an alternative.
More broadly, JSON schema validation isn't something unique and other modules could benefit from such a helper that accepts a schema
I like the idea; ideally, such schemas should be included in the main binary (similar to opencode) to ensure that configuration files are tested against the schema versions provided by the main binary, as those schemas could change over time.
| }; | ||
| validateOpenCodeThemes = { | ||
| schema = "theme"; | ||
| instances = lib.mapAttrsToList (name: _: "opencode/themes/${name}.json") cfg.themes; |
There was a problem hiding this comment.
This won't work if cfg.themes is a path to a directory.
There was a problem hiding this comment.
@delafthi, good catch! I'll fix it and add more test coverage.
| [[ "$stderr" == *"Schema validation errors"* ]] || { | ||
| printf 'Unexpected stderr:\n%s\n' "$stderr" | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
In case you add the functionality to provide additional arguments to check-jsonschema you can add --output-format=json argument to parse the output via jq. This would make it easier to check the output.
There was a problem hiding this comment.
That sounds good! I will try that.
There was a problem hiding this comment.
@delafthi I added some missing tests to cover the case where cfg.themes is a directory. Additionally, I updated the assertions to use jq to parse the errors.
26265e8 to
61255b0
Compare
8932474 to
96cb131
Compare
96cb131 to
4365fef
Compare
Description
Make use of schema metadata to validate JSON configuration files.
Checklist
Change is backwards compatible.
Code formatted with
nix fmtornix-shell -A dev --run treefmt.Code tested through
nix build .#test-allor a targeted
nix run .#tests -- <pattern>.Test cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new module
If this PR adds an exciting new feature or contains a breaking change.