Repository navigation
Conversation
See: #916 Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
| WRITE expected_file_0.txt UNTIL EOF | ||
| { | ||
| "title": "My project", | ||
| "dependencies": { |
There was a problem hiding this comment.
TODO to self: Maybe dependencies should be kept last?
jviotti
changed the title
jsonschema.jsonjsonschema.json
🤖 Augment PR SummarySummary: Adds configuration-file formatting without requiring a JSON Schema dialect.
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| if (options.contains("keep-ordering")) { | ||
| sourcemeta::jsonschema::write_schema(entry.second, expected, | ||
| indentation, entry.roundtrip); | ||
| if (is_configuration) { |
There was a problem hiding this comment.
Could you check the new branch at src/command_fmt.cc:316 with "ignore": ["./jsonschema.json"], a pattern already used in test/format/pass_cwd_config_no_path.clitest? for_each_json filters the explicitly requested configuration out at src/input.h:624–629 before this branch runs, so fmt jsonschema.json leaves it unchanged and fmt jsonschema.json --check reports success even when it is unformatted.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| // goes on to order by the configuration format rather than by any dialect, | ||
| // so none of what tells a schema from the rest has a say over it | ||
| const auto is_configuration{configuration_path.has_value() && | ||
| configuration_path.value() == |
There was a problem hiding this comment.
Could you check the comparison at src/command_fmt.cc:296–298 when jsonschema.json is a symlink to a same-directory config.json? for_each_json canonicalizes entry.resolution_base to the target, but Configuration::find returns the symlink path, so is_configuration is false and fmt jsonschema.json takes the schema path, failing without a default dialect or applying the wrong ordering.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
@cubic-dev-ai review |
@jviotti I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/command_fmt.cc">
<violation number="1" location="src/command_fmt.cc:317">
P3: This message prints unconditionally, so a passing `jsonschema fmt --check jsonschema.json` in CI now always emits a line on stderr even without `--verbose`, polluting otherwise-silent check output. Guard it with `LOG_VERBOSE(options)` like the surrounding check/format progress logs, or emit it only in the non-check path.</violation>
</file>
<file name="src/utils.h">
<violation number="1" location="src/utils.h:200">
P2: Formatting a configuration file no longer validates it. Previously `fmt` always ran `read_configuration` -> blaze `Configuration::from_json` on the entry, so a malformed `jsonschema.json` (non-object root, or `extension`/`ignore`/`exclude` with non-string members) was rejected with a positioned `ConfigurationParseError`. The new `is_configuration` branch in `command_fmt.cc` skips that path entirely and just runs `format_configuration` plus `write_schema`, so such documents are silently reordered/rewritten (and `--check` passes on them). As a secondary risk, this `std::sort` compares heterogeneous `JSON` values via `JSON::operator<` (`sourcemeta::core::JSON::operator<`, vendor/core `json_value.h`), which core only defines for comparable values and which can throw, crashing `fmt` on arrays mixing types. Validate the document as a configuration (e.g., call `read_configuration` and surface `ConfigurationParseError`) before formatting it as one, or at minimum bail out of sorting when the array contains non-string elements.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| return; | ||
| } | ||
|
|
||
| std::sort(value.as_array().begin(), value.as_array().end()); |
There was a problem hiding this comment.
P2: Formatting a configuration file no longer validates it. Previously fmt always ran read_configuration -> blaze Configuration::from_json on the entry, so a malformed jsonschema.json (non-object root, or extension/ignore/exclude with non-string members) was rejected with a positioned ConfigurationParseError. The new is_configuration branch in command_fmt.cc skips that path entirely and just runs format_configuration plus write_schema, so such documents are silently reordered/rewritten (and --check passes on them). As a secondary risk, this std::sort compares heterogeneous JSON values via JSON::operator< (sourcemeta::core::JSON::operator<, vendor/core json_value.h), which core only defines for comparable values and which can throw, crashing fmt on arrays mixing types. Validate the document as a configuration (e.g., call read_configuration and surface ConfigurationParseError) before formatting it as one, or at minimum bail out of sorting when the array contains non-string elements.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/utils.h, line 200:
<comment>Formatting a configuration file no longer validates it. Previously `fmt` always ran `read_configuration` -> blaze `Configuration::from_json` on the entry, so a malformed `jsonschema.json` (non-object root, or `extension`/`ignore`/`exclude` with non-string members) was rejected with a positioned `ConfigurationParseError`. The new `is_configuration` branch in `command_fmt.cc` skips that path entirely and just runs `format_configuration` plus `write_schema`, so such documents are silently reordered/rewritten (and `--check` passes on them). As a secondary risk, this `std::sort` compares heterogeneous `JSON` values via `JSON::operator<` (`sourcemeta::core::JSON::operator<`, vendor/core `json_value.h`), which core only defines for comparable values and which can throw, crashing `fmt` on arrays mixing types. Validate the document as a configuration (e.g., call `read_configuration` and surface `ConfigurationParseError`) before formatting it as one, or at minimum bail out of sorting when the array contains non-string elements.</comment>
<file context>
@@ -143,6 +148,115 @@ inline auto looks_like_test_document(const sourcemeta::core::JSON &document)
+ return;
+ }
+
+ std::sort(value.as_array().begin(), value.as_array().end());
+}
+
</file context>
| sourcemeta::jsonschema::write_schema(entry.second, expected, | ||
| indentation, entry.roundtrip); | ||
| if (is_configuration) { | ||
| std::cerr << "Interpreting as a configuration file: " << entry.first |
There was a problem hiding this comment.
P3: This message prints unconditionally, so a passing jsonschema fmt --check jsonschema.json in CI now always emits a line on stderr even without --verbose, polluting otherwise-silent check output. Guard it with LOG_VERBOSE(options) like the surrounding check/format progress logs, or emit it only in the non-check path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/command_fmt.cc, line 317:
<comment>This message prints unconditionally, so a passing `jsonschema fmt --check jsonschema.json` in CI now always emits a line on stderr even without `--verbose`, polluting otherwise-silent check output. Guard it with `LOG_VERBOSE(options)` like the surrounding check/format progress logs, or emit it only in the non-check path.</comment>
<file context>
@@ -296,30 +312,43 @@ auto sourcemeta::jsonschema::fmt(const sourcemeta::core::Options &options)
- sourcemeta::jsonschema::write_schema(entry.second, expected,
- indentation, entry.roundtrip);
+ if (is_configuration) {
+ std::cerr << "Interpreting as a configuration file: " << entry.first
+ << "\n";
+ if (options.contains("keep-ordering")) {
</file context>
| std::cerr << "Interpreting as a configuration file: " << entry.first | |
| LOG_VERBOSE(options) << "Interpreting as a configuration file: " | |
| << entry.first << "\n"; |
There was a problem hiding this comment.
All reported issues were addressed across 17 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Re-trigger cubic
This branch has not been deployed
1 participant
See: #916
Signed-off-by: Juan Cruz Viotti jv@jviotti.com