Skip to content

fix(compliance-audit): use null-safe jq for boolean settings checks#131

Open
don-petry wants to merge 104 commits into
mainfrom
fix/compliance-audit-jq-boolean
Open

fix(compliance-audit): use null-safe jq for boolean settings checks#131
don-petry wants to merge 104 commits into
mainfrom
fix/compliance-audit-jq-boolean

Conversation

@don-petry
Copy link
Copy Markdown
Contributor

@don-petry don-petry commented Apr 14, 2026

Summary

  • Fix jq // operator bug that caused has_wiki: false to always be reported as current: null
  • jq's // (alternative) operator treats false as falsy — so false // "null" returns "null" instead of "false"
  • This caused any boolean check with expected value false (e.g. has_wiki) to perpetually flag as non-compliant even when correctly set

Root Cause

Line 303 in scripts/compliance-audit.sh:

Before (broken for false values):
actual=$(echo "$settings" | jq -r ".$key // "null"")

After (null-safe):
actual=$(echo "$settings" | jq -r "if .$key == null then "null" else (.$key | tostring) end")

Verification

echo '{"has_wiki": false}' | jq -r 'if .has_wiki == null then "null" else (.has_wiki | tostring) end'
=> false (correct)

echo '{"has_wiki": false}' | jq -r '.has_wiki // "null"'
=> null (broken — was causing the false positive)

Closes petry-projects/ContentTwin#63

Generated with Claude Code

Summary by CodeRabbit

  • Chores
    • Improved reliability of repository settings handling so compliance checks and settings application are more consistent and less prone to formatting quirks.
    • Reduced incidental output differences and false positives by stabilizing how setting values (including default branch and merge message fields) are read and parsed.

jq's // operator treats false as falsy, so false // "null" returns
"null" rather than "false". This caused boolean settings checks with
expected value of false (e.g. has_wiki) to always report a compliance
finding even when the setting was correctly set to false.

Replace the // "null" fallback with an explicit null test:
  if .$key == null then "null" else (.$key | tostring) end

This correctly returns "false" for a false value and "null" only when
the field is actually absent.

Closes petry-projects/ContentTwin#63
Copilot AI review requested due to automatic review settings April 14, 2026 14:11
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 14, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Normalized how JSON is passed into jq across two scripts: replaced echo "$..." with printf '%s' "$...", and changed per-key lookups to pass the key with --arg key "$key" and use indexed access .[$key] in jq filters. Changes affect scripts/compliance-audit.sh and scripts/apply-repo-settings.sh.

Changes

jq input and key-lookup normalization

Layer / File(s) Summary
Data Input Normalization
scripts/compliance-audit.sh, scripts/apply-repo-settings.sh
Replace echo "$settings" / echo "$current" with printf '%s' "$settings" / printf '%s' "$current" when piping JSON into jq.
Key Lookup Change
scripts/compliance-audit.sh
Per-key lookup now uses jq -r --arg key "$key" '.[$key]' instead of string-interpolating the key into the jq filter.
Default Branch Extraction
scripts/compliance-audit.sh
default_branch is read by piping printf '%s' "$settings" into jq -r '.default_branch' (replaces prior echo-based pipeline).
Per-field Reads
scripts/apply-repo-settings.sh
Reading of actual values and squash_merge_commit_title / squash_merge_commit_message now uses printf-piped JSON into jq (replaces echo usage).
Tests / Docs
(none changed)
No tests or documentation modified.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the core fix: using null-safe jq logic to properly handle boolean settings that are false, resolving the false-positive reporting issue.
Linked Issues check ✅ Passed The PR directly addresses issue #63 by fixing the jq boolean settings bug; actual changes use null-safe jq patterns (if .key == null checks) and printf for consistency, fulfilling the stated objective.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the jq boolean settings bug described in issue #63; both scripts apply consistent null-safe jq patterns and printf improvements with no extraneous changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/compliance-audit-jq-boolean

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@don-petry
Copy link
Copy Markdown
Contributor Author

@don-petry This PR fixes the root cause of the compliance finding in petry-projects/ContentTwin#63. The ContentTwin wiki is already correctly set to false via the GitHub API — the audit script had a jq bug where false // "null" returns "null" (jq's // operator treats false as falsy). Please review and merge when CI is green.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR fixes compliance audit false-positives for boolean repo settings (notably when the correct value is false) by making the jq extraction null-safe instead of using jq’s // operator.

Changes:

  • Replace jq // "null" fallback with an explicit null check and tostring conversion for boolean settings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread scripts/compliance-audit.sh Outdated
Comment thread scripts/compliance-audit.sh Outdated
@don-petry
Copy link
Copy Markdown
Contributor Author

@claude[agent] - Fix merge conflicts

- Merge PR #133 from main (same jq boolean fix)
- Apply printf instead of echo for JSON piping (safer)
- Use jq --arg for key interpolation (prevents injection)

Agent-Logs-Url: https://github.com/petry-projects/.github/sessions/bc09d7ce-9add-488c-a416-223d826cc900

Co-authored-by: don-petry <36422719+don-petry@users.noreply.github.com>
@Claude
Copy link
Copy Markdown

Claude AI commented Apr 15, 2026

@claude[agent] - Fix merge conflicts

Merge conflicts resolved in commit 5358531. The resolution merges PR #133 from main (which had the same boolean fix with cleaner jq syntax) and applies both Copilot review suggestions:

  1. Use printf '%s' instead of echo for JSON piping (safer, prevents escape issues)
  2. Use jq --arg key "$key" with .[$key] for safe key interpolation (prevents injection with special characters)

All changes tested and verified working correctly.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/graphql
    • Triggering command: /usr/bin/gh gh repo view petry-projects/.github --json defaultBranchRef --jq .defaultBranchRef.name /home/REDACTED/.nvm/nvm.sh /home/REDACTED/.nvm/package.json /home/REDACTED/.nvm/rename_test.sh /home/REDACTED/.nvm/test ARGV�� --abbrev-ref HEAD /usr/bin/awk (http block)

If you need me to access, download, or install something from one of these locations, you can either:

coderabbitai[bot]
coderabbitai Bot previously approved these changes Apr 15, 2026
@don-petry
Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request

…intf and --arg key

Co-authored-by: don-petry <36422719+don-petry@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI commented Apr 16, 2026

@copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in c10108f. The conflict in scripts/compliance-audit.sh was between main's echo "$settings" | jq -r ".$key | ..." form and our branch's safer printf '%s' "$settings" | jq -r --arg key "$key" '.[$key] | ...' form — kept the safer version from this PR.

Copy link
Copy Markdown
Contributor Author

@don-petry don-petry left a comment

Choose a reason for hiding this comment

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

Automated review — APPROVED

Risk: LOW
Reviewed commit: 07060da8ae3a4246d6b830ca73cac46687248652
Cascade: triage → deep (see triage: haiku 4.5 → deep: sonnet 4.6 + duck: gpt-5.4 → audit: opus 4.6 for models)

Summary

Single-file shell script fix replacing echo with printf '%s' and dynamic jq key interpolation with --arg/.[$key] for safer jq invocation. All CI checks pass (ShellCheck, Lint, SonarCloud quality gate, CodeQL, Agent Security Scan). The triage escalation was caused by a triage-tier failure, not a real risk signal — the change itself is a low-risk correctness and hardening improvement.

Findings

Info — Correctness

  • scripts/compliance-audit.sh:305 — Replaces echo | jq -r '.$key ...' with printf '%s' | jq -r --arg key '$key' '.[$key] ...'. Using printf avoids echo flag edge-cases (-e, -n) when $settings contains leading-dash or backslash content, and --arg/.[$key] prevents jq filter injection if a key name ever contains special characters. Minor but strictly better.
  • scripts/compliance-audit.sh:315 — Same echoprintf change for default_branch extraction. No functional difference for a static field path, but consistent with the pattern above.

Info — CI

  • All checks green: Lint, ShellCheck, Agent Security Scan, CodeQL (actions), SonarCloud (0 new issues, 0 security hotspots), CodeRabbit. No failures or warnings.

Info — Triage

  • Triage escalation reason was triage-output-invalid — the triage tier failed to produce output, not a genuine risk signal in this PR. No HIGH-risk patterns (auth/secrets/credentials/crypto, DB migrations, injection, hardcoded secrets, CI tampering) were present.

CI status

All checks green: Lint, ShellCheck, Agent Security Scan, CodeQL (actions), SonarCloud (0 new issues, 0 security hotspots), CodeRabbit. No failures or warnings.


Reviewed by the don-petry PR-review cascade (triage: haiku 4.5 → deep: sonnet 4.6 + duck: gpt-5.4 → audit: opus 4.6). Reply with @don-petry if you need a human.

@don-petry don-petry enabled auto-merge (squash) April 16, 2026 13:14
@don-petry
Copy link
Copy Markdown
Contributor Author

don-petry commented Apr 16, 2026

Outdated review (superseded by re-review at b939ca392b44d3d7bd376d4e0a2bb988dcc567a0) — click to expand.

Automated review — APPROVED

Risk: LOW
Reviewed commit: d43f2944bb56ba96c1de7e55703e63792cf49383
Cascade: triage → deep (see triage: haiku 4.5 → deep: sonnet 4.6 + duck: gpt-5.4 → audit: opus 4.6 for models)

Note: GitHub does not allow self-approval. Review posted as a comment; auto-merge is being requested below.

Summary

Single-file shell script hardening: replaces echo with printf '%s' and switches from dynamic jq filter string interpolation (.$key) to parameterized key lookup (--arg key / .[$key]), which is strictly safer. All CI checks pass (ShellCheck, Lint, AgentShield, CodeQL, SonarCloud 0 new issues/hotspots, CodeRabbit); the triage escalation was caused by a triage-tier output failure, not any genuine risk signal in the PR itself.

Findings

Info

  • [info] scripts/compliance-audit.sh:305printf '%s' is safer than echo for piping JSON: avoids misinterpretation of leading-dash or backslash content on shells where echo processes flags/escapes.
  • [info] scripts/compliance-audit.sh:305 — Using --arg key "$key" with .[$key] prevents jq filter injection if a key name ever contains special characters (], |, etc.). Strictly better than embedding $key directly in the filter string.
  • [info] scripts/compliance-audit.sh:315echoprintf '%s' change for default_branch extraction is a consistency improvement; no functional impact for a static field path.
  • [info] (CI) — All checks green: Lint, ShellCheck, AgentShield, CodeQL (actions), SonarCloud (0 new issues, 0 security hotspots), CodeRabbit. No failures or warnings.
  • [info] (Triage) — Triage escalation reason was triage-output-invalid — the triage tier failed to produce output, not a genuine risk signal in this PR. No HIGH-risk patterns (auth/secrets/credentials/crypto, DB migrations, injection, hardcoded secrets, CI tampering) are present.
  • [info] (Re-review) — Prior review approved at SHA 07060da8; current head d43f2944 is a no-op merge-main-into-branch commit — actual code changes are identical to the previously approved state.

CI status

All checks green: Lint, ShellCheck, AgentShield, CodeQL, SonarCloud (0 new issues, 0 security hotspots), CodeRabbit.


Reviewed by the don-petry PR-review cascade (triage: haiku 4.5 → deep: sonnet 4.6 + duck: gpt-5.4 → audit: opus 4.6). Reply with @don-petry if you need a human.

@don-petry
Copy link
Copy Markdown
Contributor Author

don-petry commented Apr 16, 2026

Outdated review (superseded by re-review at b939ca392b44d3d7bd376d4e0a2bb988dcc567a0) — click to expand.

Automated review — APPROVED

Risk: LOW
Reviewed commit: 8226d0f2b1957400ad08e31d9f9c9762eb1b0e79
Cascade: triage → deep (see triage: haiku 4.5 → deep: sonnet 4.6 + duck: gpt-5.4 → audit: opus 4.6 for models)

Summary

Single-file shell script hardening in compliance-audit.sh: replaces echo with printf '%s' for safer JSON piping and switches from dynamic jq key interpolation (.$key) to parameterized access (--arg key / .[$key]), which is strictly safer against jq filter injection. All CI checks pass (Lint, ShellCheck, AgentShield, CodeQL, SonarCloud 0 new issues/hotspots, CodeRabbit). The triage escalation was caused by a triage-tier output failure, not any genuine risk signal in this PR; a prior deep-review cycle at SHA d43f294 already approved the same change as LOW risk, and the current head (8226d0f) is a no-op merge-of-main commit with no substantive code delta.

Findings

Info (informational only — no action required)

  • [info/correctness] scripts/compliance-audit.sh:305printf '%s' is safer than echo for piping JSON: avoids misinterpretation of leading-dash content or backslash sequences on shells where echo processes flags/escapes.
  • [info/security-hardening] scripts/compliance-audit.sh:305 — Using --arg key "$key" with .[$key] prevents jq filter injection if a key name ever contains special characters (], |, etc.). Strictly better than embedding $key directly in the filter string.
  • [info/correctness] scripts/compliance-audit.sh:315echoprintf '%s' change for default_branch extraction is a consistency improvement; no functional impact for a static field path.
  • [info/triage] Triage escalation reason was triage-output-invalid — the triage tier failed to produce output, not a genuine risk signal in this PR. No HIGH-risk patterns present (no auth/secrets/credentials/crypto, no DB migrations, no injection introduced, no hardcoded secrets, no CI workflow tampering).
  • [info/re-review] Prior deep-review cycle approved at SHA d43f294; current head 8226d0f is a no-op merge-of-main commit — actual code changes are identical to the previously approved state.

CI status

All checks green: Lint, ShellCheck, AgentShield, CodeQL (actions), SonarCloud (0 new issues, 0 security hotspots), CodeRabbit, Agent Security Scan, Dependency audit. No failures or warnings.


Reviewed by the don-petry PR-review cascade (triage: haiku 4.5 → deep: sonnet 4.6 + duck: gpt-5.4 → audit: opus 4.6). Reply with @don-petry if you need a human.

@don-petry
Copy link
Copy Markdown
Contributor Author

@dev-lead - please fix this PR

@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compliance: has_wiki

5 participants