t2929: Fix config-helper.sh _jsonc_get discards false values#2934
t2929: Fix config-helper.sh _jsonc_get discards false values#2934marcusquinn wants to merge 1 commit intomainfrom
Conversation
… empty Replace jq's `// empty` alternative operator with explicit null checks in bash. The `//` operator in jq treats both `null` and `false` as falsy, so `false // empty` returns `empty` — silently discarding the value and falling through to the default (typically `true`). This made it impossible to disable any boolean config via config.jsonc. The env var override path worked because it bypasses _jsonc_get entirely. Fixed in 5 locations: - _jsonc_get(): core getter used by config_get/config_enabled - _jsonc_get_raw(): raw file getter - cmd_list(): user override display (false showed as default) - cmd_set(): default validation (false defaults rejected as unknown) - cmd_list source detection: used -n on user_val instead of raw null check Closes #2929
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug where Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe script was updated to properly distinguish between absent JSON keys and explicit falsy values (false, 0). The jq Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a critical bug where false and 0 values in JSONC configuration were being discarded. The approach of fetching the raw value and explicitly checking for null in bash is robust and solves the problem effectively. My review includes a few suggestions to improve debuggability by removing stderr suppression, in line with the repository's general guidelines. Overall, this is a great fix.
| # it treats `false` and `null` the same (both are falsy in jq). Instead, | ||
| # get the raw JSON value and check for null explicitly in bash. | ||
| local raw_value | ||
| raw_value=$(echo "$merged" | jq --arg p "$dotpath" 'getpath($p | split("."))' 2>/dev/null) || raw_value="null" |
There was a problem hiding this comment.
This correctly fixes the issue with false values. However, using 2>/dev/null suppresses potentially useful error messages from jq, for instance if the configuration file contains invalid JSON. This goes against a general rule for this repository. Since you're already handling the error case with || raw_value="null", you can safely remove the stderr redirection to improve debuggability.
| raw_value=$(echo "$merged" | jq --arg p "$dotpath" 'getpath($p | split("."))' 2>/dev/null) || raw_value="null" | |
| raw_value=$(echo "$merged" | jq --arg p "$dotpath" 'getpath($p | split("."))') || raw_value="null" |
References
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.
| echo "$json" | jq -r --arg p "$dotpath" 'getpath($p | split(".")) // empty' 2>/dev/null || echo "" | ||
| # Avoid `// empty` — it discards false and 0 (falsy in jq). Check for null explicitly. | ||
| local raw_value | ||
| raw_value=$(echo "$json" | jq --arg p "$dotpath" 'getpath($p | split("."))' 2>/dev/null) || raw_value="null" |
There was a problem hiding this comment.
Similar to the previous comment, removing 2>/dev/null here will improve debuggability by allowing jq errors (e.g., from malformed JSON) to be visible, while the || raw_value="null" construct gracefully handles the failure.
| raw_value=$(echo "$json" | jq --arg p "$dotpath" 'getpath($p | split("."))' 2>/dev/null) || raw_value="null" | |
| raw_value=$(echo "$json" | jq --arg p "$dotpath" 'getpath($p | split("."))') || raw_value="null" |
References
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.
| user_val=$(echo "$user_json" | jq -r --arg p "$dotpath" 'getpath($p | split(".")) // empty' 2>/dev/null) || user_val="" | ||
| # Get raw JSON value to distinguish null (absent) from false/0 | ||
| local raw_user_val | ||
| raw_user_val=$(echo "$user_json" | jq --arg p "$dotpath" 'getpath($p | split("."))' 2>/dev/null) || raw_user_val="null" |
There was a problem hiding this comment.
To align with repository guidelines and improve debugging, please remove the 2>/dev/null. This will allow errors from jq to be reported if $user_json is malformed, which can be very helpful, while still falling back to the null value as intended.
| raw_user_val=$(echo "$user_json" | jq --arg p "$dotpath" 'getpath($p | split("."))' 2>/dev/null) || raw_user_val="null" | |
| raw_user_val=$(echo "$user_json" | jq --arg p "$dotpath" 'getpath($p | split("."))') || raw_user_val="null" |
References
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.
| local default_val | ||
| default_val=$(echo "$defaults_json" | jq -r --arg p "$dotpath" 'getpath($p | split(".")) // empty' 2>/dev/null) || default_val="" | ||
| local raw_default_val default_val | ||
| raw_default_val=$(echo "$defaults_json" | jq --arg p "$dotpath" 'getpath($p | split("."))' 2>/dev/null) || raw_default_val="null" |
There was a problem hiding this comment.
Please remove the 2>/dev/null here as well. Allowing jq to report syntax errors from the defaults file on stderr will make it easier to diagnose issues with the default configuration, without affecting the script's logic thanks to the || fallback.
| raw_default_val=$(echo "$defaults_json" | jq --arg p "$dotpath" 'getpath($p | split("."))' 2>/dev/null) || raw_default_val="null" | |
| raw_default_val=$(echo "$defaults_json" | jq --arg p "$dotpath" 'getpath($p | split("."))') || raw_default_val="null" |
References
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.
1 participant
Summary
// emptyalternative operator with explicit null checks in bash across 5 locations inconfig-helper.shfalseand0values are now correctly preserved instead of being silently discarded and replaced with defaultssupervisor_pulse: false) can now be disabled viaconfig.jsoncas intendedRoot Cause
jq's
//(alternative) operator treats bothnullandfalseas falsy. Sofalse // emptyreturnsempty, which becomes an empty string in bash, triggering the default fallback. This affected_jsonc_get,_jsonc_get_raw,cmd_list, andcmd_set.Fix
Get the raw JSON value (without
-ror// empty), check for literal"null"string in bash, then render withjq -r '.'only for non-null values. This correctly distinguishesnull(key absent) fromfalse/0(key present with falsy value).Verification
Tested all paths:
_jsonc_get,_jsonc_get_raw,config_get,config_enabled,is_feature_enabled— all correctly returnfalsefor false values and fall through to defaults only for absent keys. ShellCheck clean (no new violations).Closes #2929
Summary by CodeRabbit