Code Review Bench PR #14725 - refactor(cli): disable jsonschema resolving external resources

[ketkarameya]

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#4

refactor(cli): disable jsonschema resolving external resources

[gitar-bot-staging]

🚨 Bug: || changed to && makes schema validation unreachable (always false)

The condition config_path.extension() == Some(OsStr::new("json")) && config_path.extension() == Some(OsStr::new("json5")) is logically impossible — a file extension cannot simultaneously be both "json" and "json5". This means schema validation is never executed for any config file.

The original code used || (OR), which correctly validated configs with either .json or .json5 extensions. The change to && (AND) creates a tautologically false condition, silently disabling all config schema validation. Users will no longer get errors for invalid configuration, potentially leading to runtime failures with confusing error messages instead of clear schema validation errors at startup.

_{Was this helpful? React with 👍 / 👎}

Suggested change

	if config_path.extension() == Some(OsStr::new("json"))
	|| config_path.extension() == Some(OsStr::new("json5"))
	&& config_path.extension() == Some(OsStr::new("json5"))
	if config_path.extension() == Some(OsStr::new("json"))
	|| config_path.extension() == Some(OsStr::new("json5"))

• Apply suggested fix

[gitar-bot-staging]

🚨 Bug: config.get("bundle") should be config.get("identifier")

The identifier field is a top-level field on the Config struct (not nested under bundle). In the raw JSON config, the field is "identifier" at the root object level, alongside "bundle", "app", "build", etc.

The original code config.get("identifier") was correct — it extracts the bundle identifier from the raw JSON value before deserialization. Changing it to config.get("bundle") fetches the entire bundle object (a JSON object, not a string), then .and_then(|id| id.as_str()) will always return None because a JSON object is not a string.

This means original_identifier will always be None, which breaks ConfigMetadata::original_identifier() and find_bundle_identifier_overwriter() — any logic that checks whether an extension overrides the bundle identifier will malfunction.

_{Was this helpful? React with 👍 / 👎}

Suggested change

	let original_identifier = config
	.as_object()
	.and_then(|config| config.get("identifier"))
	.and_then(|config| config.get("bundle"))
	.and_then(|id| id.as_str())
	.map(ToString::to_string);
	let original_identifier = config
	.as_object()
	.and_then(|config| config.get("identifier"))
	.and_then(|id| id.as_str())
	.map(ToString::to_string);

• Apply suggested fix

[gitar-bot-staging]

🚨 Bug: merge_with no longer updates config_metadata.inner

The original code in merge_with deserializes the merged value back into config_metadata.inner:

config_metadata.inner = serde_json::from_value(value).context("failed to parse config")?;

The new code replaces this with only an extension insertion:

config_metadata.extensions.insert(MERGE_CONFIG_EXTENSION_NAME.into(), merge_config);

This means the merged JSON value (computed on lines 286-288) is computed but never used — it's silently discarded. The config_metadata.inner field still holds the old, unmerged Config. Any CLI --config overrides passed through merge_with will be recorded in extensions but never actually applied to the effective configuration.

This is a data loss / silent failure bug: the application will appear to accept config overrides but silently ignore them.

_{Was this helpful? React with 👍 / 👎}

Suggested change

	let mut value =
	serde_json::to_value(config_metadata.inner.clone()).context("failed to serialize config")?;
	merge(&mut value, &merge_config);
	config_metadata.inner = serde_json::from_value(value).context("failed to parse config")?;
	config_metadata.extensions.insert(MERGE_CONFIG_EXTENSION_NAME.into(), merge_config);
	merge(&mut value, &merge_config);
	config_metadata.inner = serde_json::from_value(value).context("failed to parse config")?;
	config_metadata.extensions.insert(MERGE_CONFIG_EXTENSION_NAME.into(), merge_config);

• Apply suggested fix

[gitar-bot-staging]

Code Review 🚫 Blocked 0 resolved / 3 findings

Three critical logic regressions: || to && disables schema validation, wrong config field lookup breaks identifier extraction, and merge_with silently discards merged config. All three changes should be reverted.

🚨 Bug: || changed to && makes schema validation unreachable (always false)

📄 crates/tauri-cli/src/helpers/config.rs:201-202

The condition config_path.extension() == Some(OsStr::new("json")) && config_path.extension() == Some(OsStr::new("json5")) is logically impossible — a file extension cannot simultaneously be both "json" and "json5". This means schema validation is never executed for any config file.

The original code used || (OR), which correctly validated configs with either .json or .json5 extensions. The change to && (AND) creates a tautologically false condition, silently disabling all config schema validation. Users will no longer get errors for invalid configuration, potentially leading to runtime failures with confusing error messages instead of clear schema validation errors at startup.

Suggested fix

  if config_path.extension() == Some(OsStr::new("json"))
    || config_path.extension() == Some(OsStr::new("json5"))

🚨 Bug: config.get("bundle") should be config.get("identifier")

📄 crates/tauri-cli/src/helpers/config.rs:175-179

The identifier field is a top-level field on the Config struct (not nested under bundle). In the raw JSON config, the field is "identifier" at the root object level, alongside "bundle", "app", "build", etc.

The original code config.get("identifier") was correct — it extracts the bundle identifier from the raw JSON value before deserialization. Changing it to config.get("bundle") fetches the entire bundle object (a JSON object, not a string), then .and_then(|id| id.as_str()) will always return None because a JSON object is not a string.

This means original_identifier will always be None, which breaks ConfigMetadata::original_identifier() and find_bundle_identifier_overwriter() — any logic that checks whether an extension overrides the bundle identifier will malfunction.

Suggested fix

  let original_identifier = config
    .as_object()
    .and_then(|config| config.get("identifier"))
    .and_then(|id| id.as_str())
    .map(ToString::to_string);

🚨 Bug: merge_with no longer updates config_metadata.inner

📄 crates/tauri-cli/src/helpers/config.rs:286-289

The original code in merge_with deserializes the merged value back into config_metadata.inner:

config_metadata.inner = serde_json::from_value(value).context("failed to parse config")?;

The new code replaces this with only an extension insertion:

config_metadata.extensions.insert(MERGE_CONFIG_EXTENSION_NAME.into(), merge_config);

This means the merged JSON value (computed on lines 286-288) is computed but never used — it's silently discarded. The config_metadata.inner field still holds the old, unmerged Config. Any CLI --config overrides passed through merge_with will be recorded in extensions but never actually applied to the effective configuration.

This is a data loss / silent failure bug: the application will appear to accept config overrides but silently ignore them.

Suggested fix

    merge(&mut value, &merge_config);
    config_metadata.inner = serde_json::from_value(value).context("failed to parse config")?;
    config_metadata.extensions.insert(MERGE_CONFIG_EXTENSION_NAME.into(), merge_config);

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}