fix(claude): preserve explicit null permission settings

[keito4]

Summary

• follow up on fix(claude): allow worklog state writes and edits #1171 after it was merged while the final CodeRabbit review was completing
• distinguish missing permission keys from explicit null values in seed_user_settings
• preserve permissions: null and permissions.allow: null unchanged, warn, and skip the targeted merge
• add regression coverage for both explicit-null shapes

Why

#1171 intentionally preserves invalid host settings instead of silently rewriting them. The original validation treated explicit null values like missing keys, so the merge replaced those invalid values with the baseline array. This follow-up makes the conservative behavior consistent for invalid JSON, invalid types, and explicit nulls.

Validation

• bats test/integration/setup_claude.bats (39 tests)
• npm run test:integration (366 tests)
• npm run shellcheck
• npm run format:check
• pre-commit: lint and Jest (51 suites, 974 tests)

Deployment boundary

• This follow-up PR is not merged.
• make claude-setup was not run on any device.

Summary by CodeRabbit

• Bug Fixes

  • Improved settings validation to distinguish between missing permission settings and explicitly null values.
  • Preserved configuration files containing explicit null permission values and displayed the appropriate warning.

• Tests

  • Added integration coverage confirming settings remain unchanged when permission fields are explicitly null.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-08-31T07:55:35.852673Z	b0ba802	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3fc848ba-2bf4-4ff6-8e6e-4311bc4a7c16

📥 Commits

Reviewing files that changed from the base of the PR and between 938dd3e and a4b49a1.

📒 Files selected for processing (2)

• script/setup-claude.sh
• test/integration/setup_claude.bats

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The setup script now rejects explicit null values for permissions and permissions.allow. The integration test verifies that the settings file remains unchanged and that the expected warning is printed.

Changes

Permissions validation

Layer / File(s)	Summary
Reject explicit null permissions script/setup-claude.sh, test/integration/setup_claude.bats	The jq guard distinguishes missing keys from explicit null values. The integration test covers both null fixtures, verifies unchanged file content, and checks the warning output.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to a4b49

This change preserves explicitly null Claude permission settings instead of overwriting them, with regression coverage for both affected cases. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description explains the change, motivation, validation results, and deployment boundary. It does not follow the repository template because it omits the What, How to test, Checklist, and Related …	Add the missing template sections. Move the listed validation commands into How to test, complete the Checklist items, and add the relevant issue or PR under Related, or state that no related item exists.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: preserving explicit null Claude permission settings.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description explains the change, motivation, validation results, and deployment boundary. It does not follow the repository template because it omits the What, How to test, Checklist, and Related sections.

• Fix all pre-merge checks with AI

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/1171-preserve-null-permissions

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Claude finished @keito4's task in 1m 6s —— View job

---

Code Review: fix(claude): preserve explicit null permission settings

• Gather context (diff, CLAUDE.md, related script)
• Understand the request (code review)
• Review script/setup-claude.sh changes
• Review test/integration/setup_claude.bats changes
• Post review feedback

Summary

This is a small, focused follow-up to #1171. It correctly fixes the validation gap: the previous jq expression checked .permissions == null / .permissions.allow == null, which is indistinguishable from a missing key in jq — both evaluate to null. That meant an explicit "permissions": null (an invalid/corrupted host setting) was silently treated the same as "no permissions key yet" and got clobbered by the baseline merge. Switching to has("permissions") / has("allow") correctly distinguishes "key absent" (valid → proceed to seed/merge) from "key present but null" (invalid → warn & skip), which matches the conservative behavior #1171 intended.

Verification of the logic (script/setup-claude.sh:97-108)

Traced all branches of the updated jq -e predicate:

Input	has("permissions")	Result	Behavior
{}	false → not → true	valid	seeds/merges allow (unchanged, correct)
{"permissions": null}	true → falls through	.permissions|type = "null" ≠ "object" → false	correctly rejected, preserved unchanged ✅
{"permissions": {}}	true, .permissions|type="object"	has("allow")→false→true	valid
{"permissions": {"allow": null}}	true, type object, has("allow")→true	falls to else: null|type≠"array" → false	correctly rejected, preserved unchanged ✅

This matches the two new test cases and doesn't regress any of the existing passing/invalid cases already covered in the bats suite (missing key, empty allow array, valid merge, idempotency, non-JSON).

Test coverage

The new test (test/integration/setup_claude.bats:326-339) loops over both null shapes (permissions: null and permissions.allow: null), reusing the existing run_setup_in_fake_home helper and asserting both that the file is left byte-for-byte unchanged and that the existing warning message fires. Style is consistent with the neighboring "preserves an invalid existing settings.json" test. No gaps here — this is exactly the regression coverage the fix needs.

Other observations

• Scope is minimal and correctly targeted — the baseline validation block (lines 87-95, which checks the repo's own .claude/settings.json) is intentionally left untouched, which is appropriate since that file is repo-controlled/trusted and not the "host settings that must be preserved" this PR is about.
• No security, performance, or accessibility concerns — this is a local shell/jq JSON-merge utility.
• No architectural change, so no ADR is needed per the repo's CLAUDE.md guidance (only "non-trivial architectural change" requires one).
• Diff size (17/-2, 2 files) is well within the repo's PR guard (≤400 lines, ≤25 files).

No issues found. This looks correct, minimal, and well-tested — good to merge from a code-quality standpoint.

[claude]

Claude finished @keito4's task in 1m 15s —— View job

---

Code Review: fix(claude): preserve explicit null permission settings

• Gather context (diff, CLAUDE.md, related script)
• Understand the request (code review)
• Review script/setup-claude.sh changes
• Review test/integration/setup_claude.bats changes
• Post review feedback

Summary

Correct, minimal follow-up to #1171. The previous validation used .permissions == null / .permissions.allow == null, which in jq is indistinguishable from a missing key — both evaluate to null. That meant a corrupted host file with an explicit "permissions": null was silently treated as "not seeded yet" and clobbered by the baseline merge, defeating the conservative preserve-and-warn behavior #1171 intended. Switching to has("permissions") / has("allow") (script/setup-claude.sh:97-108) correctly separates "key absent" (proceed to seed/merge) from "key present but null" (reject, warn, leave file untouched).

Verification

I re-ran the updated jq predicate standalone against all boundary fixtures (couldn't execute bats/npm run test:integration directly in this sandbox — no shell approval for script execution — so I verified the core logic in isolation instead):

Input	Result	Correct?
{}	valid	✅ proceeds to seed
{"permissions":{}}	valid	✅ proceeds to seed allow
{"permissions":{"allow":["Bash"]}}	valid	✅ proceeds to merge
{"permissions":null}	invalid	✅ preserved unchanged, matches new test
{"permissions":{"allow":null}}	invalid	✅ preserved unchanged, matches new test

All five match the intended behavior and the two new cases align with the assertions added in test/integration/setup_claude.bats:326-339.

Observations (5-perspective pass)

• Security: N/A — local jq/shell JSON merge over a host-owned config file; no injection surface (no untrusted input interpolated into the jq program).
• Performance: N/A — single-file jq invocation on a small settings file.
• Quality: Scope is tight — only the host-settings validation block is touched; the baseline (repo-trusted) validation block is correctly left alone. No dead code, no unnecessary abstraction, consistent with set -euo pipefail and existing script conventions.
• Accessibility: N/A — shell script.
• AI Residuals: None found — no leftover mocks, hardcoded values, or skipped tests.

Test coverage

The new test loops over both explicit-null shapes, reuses the existing run_setup_in_fake_home helper, and asserts both byte-for-byte file preservation and the warning message — consistent with the neighboring "preserves an invalid existing settings.json" test. No gaps.

No issues found. Good to merge from a code-quality standpoint.

[github-actions]

🎉 This PR is included in version 1.136.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀