Skip to content

t2929: Fix config-helper.sh _jsonc_get discards false values #2934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
marcusquinn wants to merge 1 commit into from
+31 −12
Closed

t2929: Fix config-helper.sh _jsonc_get discards false values #2934

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 31 additions & 12 deletions .agents/scripts/config-helper.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,13 +233,17 @@ _jsonc_get() {
merged=$(_get_merged_config)

# Use jq --arg to safely pass dotpath (no shell interpolation into filter)
local value
value=$(echo "$merged" | jq -r --arg p "$dotpath" 'getpath($p | split(".")) // empty' 2>/dev/null) || value=""
# NOTE: We must NOT use jq's `// empty` alternative operator here because
# 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"
Copy link

@gemini-code-assist gemini-code-assist bot Mar 5, 2026

Choose a reason for hiding this comment

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

medium

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.

Suggested change
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
  1. 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.


if [[ -n "$value" && "$value" != "null" ]]; then
echo "$value"
else
if [[ "$raw_value" == "null" ]]; then
echo "$default"
else
# Use jq -r to strip quotes from strings; booleans/numbers pass through as-is
echo "$raw_value" | jq -r '.'
fi
return 0
}
Expand All @@ -261,7 +265,14 @@ _jsonc_get_raw() {
echo ""
return 0
}
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"
Copy link

@gemini-code-assist gemini-code-assist bot Mar 5, 2026

Choose a reason for hiding this comment

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

medium

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.

Suggested change
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
  1. 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.

if [[ "$raw_value" == "null" ]]; then
echo ""
else
echo "$raw_value" | jq -r '.'
fi
return 0
}

Expand Down Expand Up @@ -486,7 +497,14 @@ cmd_list() {

local user_val env_val effective source

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"
Copy link

@gemini-code-assist gemini-code-assist bot Mar 5, 2026

Choose a reason for hiding this comment

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

medium

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.

Suggested change
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
  1. 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.

if [[ "$raw_user_val" == "null" ]]; then
user_val=""
else
user_val=$(echo "$raw_user_val" | jq -r '.')
fi

# Check env override
env_val=""
Expand All @@ -500,7 +518,7 @@ cmd_list() {
if [[ -n "$env_val" ]]; then
effective="$env_val"
source="env"
elif [[ -n "$user_val" ]]; then
elif [[ "$raw_user_val" != "null" ]]; then
effective="$user_val"
source="user"
else
Expand Down Expand Up @@ -579,17 +597,18 @@ cmd_set() {
# Validate dotpath contains only safe characters
_validate_dotpath "$dotpath" || return 1

# Validate key exists in defaults
# Validate key exists in defaults (use raw JSON to distinguish null from false/0)
local defaults_json
defaults_json=$(_strip_jsonc "$JSONC_DEFAULTS") || return 1
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"
Copy link

@gemini-code-assist gemini-code-assist bot Mar 5, 2026

Choose a reason for hiding this comment

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

medium

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.

Suggested change
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
  1. 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.


if [[ -z "$default_val" ]]; then
if [[ "$raw_default_val" == "null" ]]; then
echo "[ERROR] Unknown config key: $dotpath" >&2
echo " Run 'aidevops config list' to see available options." >&2
return 1
fi
default_val=$(echo "$raw_default_val" | jq -r '.')

# Validate value type from default and reject invalid input early
case "$default_val" in
Expand Down