feat(codex): persist Desktop Git settings - #2085
Conversation
Keep Codex Desktop Git and worktree preferences declarative across Home Manager activations. Preserve unrelated Desktop state while enforcing squash merges and retaining 300 worktrees. Co-Authored-By: OpenAI Codex <noreply@openai.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughCodex activation now accepts Desktop settings and ChangesCodex Desktop state integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant A as HomeActivation
participant B as activate.sh
participant C as jq
participant D as CodexGlobalState
A->>B: pass Codex settings and jq
B->>C: merge persisted Desktop state
C->>D: write temporary merged state
B->>D: atomically replace global state
Possibly related PRs
Suggested labels: Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 updates the Codex configuration activation script to support merging managed Desktop settings into the global state store using jq. It also adds a new configuration file for desktop settings, updates the Nix activation logic, and includes corresponding tests to verify the merging behavior and configuration updates. I have provided a suggestion to simplify the jq logic in the activation script by using the inputs filter to handle both existing and new state files more concisely.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if [[ -s $GLOBAL_STATE ]]; then | ||
| "$JQ_BIN" --slurpfile settings "$DESKTOP_SETTINGS_JSON" ' | ||
| .["electron-persisted-atom-state"] = | ||
| ((.["electron-persisted-atom-state"] // {}) + $settings[0]) | ||
| ' "$GLOBAL_STATE" >"$GLOBAL_STATE_TMP" | ||
| else | ||
| "$JQ_BIN" -n --slurpfile settings "$DESKTOP_SETTINGS_JSON" ' | ||
| {"electron-persisted-atom-state": $settings[0]} | ||
| ' >"$GLOBAL_STATE_TMP" | ||
| fi |
There was a problem hiding this comment.
The if/else block for handling an existing or new global state file can be simplified. By ensuring the file exists (e.g., with touch) and then using jq's inputs filter with a fallback, you can use a single jq command to handle both cases where the file is new/empty or already contains state. This removes code duplication and makes the logic more concise.
| if [[ -s $GLOBAL_STATE ]]; then | |
| "$JQ_BIN" --slurpfile settings "$DESKTOP_SETTINGS_JSON" ' | |
| .["electron-persisted-atom-state"] = | |
| ((.["electron-persisted-atom-state"] // {}) + $settings[0]) | |
| ' "$GLOBAL_STATE" >"$GLOBAL_STATE_TMP" | |
| else | |
| "$JQ_BIN" -n --slurpfile settings "$DESKTOP_SETTINGS_JSON" ' | |
| {"electron-persisted-atom-state": $settings[0]} | |
| ' >"$GLOBAL_STATE_TMP" | |
| fi | |
| touch "$GLOBAL_STATE" | |
| "$JQ_BIN" -n --slurpfile settings "$DESKTOP_SETTINGS_JSON" ' | |
| (inputs // {}) | | |
| .["electron-persisted-atom-state"] = | |
| ((.["electron-persisted-atom-state"] // {}) + $settings[0]) | |
| ' "$GLOBAL_STATE" >"$GLOBAL_STATE_TMP" |
Mesa DescriptionTL;DRPersist Codex Desktop Git and worktree settings across dotfiles activation by merging managed configuration keys into the Desktop global state while preserving unrelated user preferences. What changed?
Description generated by Mesa. Update settings |
|
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of f092df0...d0c0f39
Analysis
No significant architectural issues identified. The PR demonstrates solid engineering practices: atomic writes via temp file + mv pattern, proper state management with jq shallow merge semantics, clear separation between desktop app state and CLI config, and defensive coding patterns (permission hardening, trap cleanup). The fail-fast error handling is appropriate for configuration management. All dependencies are properly declared and the addition to the activation pipeline is backward-compatible. Test coverage validates critical behaviors including state preservation and managed key overrides.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
6 files reviewed | 3 comments | Edit Agent Settings • Read Docs
| if [[ -s $GLOBAL_STATE ]]; then | ||
| "$JQ_BIN" --slurpfile settings "$DESKTOP_SETTINGS_JSON" ' | ||
| .["electron-persisted-atom-state"] = | ||
| ((.["electron-persisted-atom-state"] // {}) + $settings[0]) |
There was a problem hiding this comment.
The shallow merge using + operator will overwrite nested objects entirely rather than deep-merging them. For example, if the existing state has {"electron-persisted-atom-state": {"a": 1, "b": 2}} and settings has {"a": 3, "c": 4}, the result will be {"a": 3, "c": 4} ("b" is lost). Consider using * (recursive merge) instead: ."electron-persisted-atom-state" = (."electron-persisted-atom-state" // {}) * $settings[0]. The current test at line 48 only validates that a specific managed key overrides an existing one, but doesn't verify that sibling unrelated keys within electron-persisted-atom-state are preserved when adding new managed keys.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#2085
File: config/codex/activate.sh#L26
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The shallow merge using `+` operator will overwrite nested objects entirely rather than deep-merging them. For example, if the existing state has `{"electron-persisted-atom-state": {"a": 1, "b": 2}}` and settings has `{"a": 3, "c": 4}`, the result will be `{"a": 3, "c": 4}` ("b" is lost). Consider using `* ` (recursive merge) instead: `."electron-persisted-atom-state" = (."electron-persisted-atom-state" // {}) * $settings[0]`. The current test at line 48 only validates that a specific managed key overrides an existing one, but doesn't verify that sibling unrelated keys within `electron-persisted-atom-state` are preserved when adding new managed keys.
| GLOBAL_STATE_TMP=$(mktemp "${GLOBAL_STATE}.tmp.XXXXXX") | ||
| trap 'rm -f "$GLOBAL_STATE_TMP"' EXIT | ||
|
|
||
| if [[ -s $GLOBAL_STATE ]]; then |
There was a problem hiding this comment.
Consider adding validation that $GLOBAL_STATE contains valid JSON before attempting the merge. If the file exists but is corrupted (e.g., truncated, manually edited), jq will fail and activation aborts. While fail-fast is reasonable, adding a backup step (e.g., cp "$GLOBAL_STATE" "$GLOBAL_STATE.backup" before the jq operation) would help users recover from accidental corruption.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#2085
File: config/codex/activate.sh#L23
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Consider adding validation that `$GLOBAL_STATE` contains valid JSON before attempting the merge. If the file exists but is corrupted (e.g., truncated, manually edited), jq will fail and activation aborts. While fail-fast is reasonable, adding a backup step (e.g., `cp "$GLOBAL_STATE" "$GLOBAL_STATE.backup"` before the jq operation) would help users recover from accidental corruption.
| @@ -0,0 +1,8 @@ | |||
| { | |||
| "git-branch-prefix": "codex/", | |||
| "git-always-force-push": true, | |||
There was a problem hiding this comment.
Setting git-always-force-push to true globally is dangerous and can lead to accidental data loss when multiple users collaborate on the same branch or when pushing to protected branches. Consider making this configurable per-repository or adding safeguards in the Git hooks to prevent force-pushing to specific branches (e.g., main, master, production).
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#2085
File: config/codex/desktop-settings.json#L3
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Setting `git-always-force-push` to `true` globally is dangerous and can lead to accidental data loss when multiple users collaborate on the same branch or when pushing to protected branches. Consider making this configurable per-repository or adding safeguards in the Git hooks to prevent force-pushing to specific branches (e.g., main, master, production).
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
|
|
||
| chmod 600 "$GLOBAL_STATE_TMP" | ||
| mv -f "$GLOBAL_STATE_TMP" "$GLOBAL_STATE" | ||
| trap - EXIT |
There was a problem hiding this comment.
Redundant trap clear: After the successful mv -f "$GLOBAL_STATE_TMP" "$GLOBAL_STATE" on line 35, the temp file has been renamed away and no longer exists, so the EXIT trap's rm -f "$GLOBAL_STATE_TMP" is already a no-op. The script is also about to exit normally, so clearing the trap adds no safety. Consider removing this line to keep the flow simpler.
| trap - EXIT |
1 participant
Summary
config.tomland preserve unrelated Desktop stateValidation
shellcheck config/codex/activate.shshellspec spec/activate_config_spec.sh(48 examples)make shell-test(1,630 ShellSpec examples and 393 Fish tests)make nix-format-checkmake nix-testmake buildsquashand300Summary by cubic
Persist Codex Desktop Git and worktree settings across Home Manager activations by merging managed keys into the Desktop global state. Enforces squash merges and keeps 300 worktrees, while preserving unrelated Desktop state.
.codex-global-state.jsonusingjq(no overwrite of unrelated state).git-pull-request-merge-method: "squash",worktree-auto-cleanup-enabled: true, andworktree-keep-count: 300.reviewDelivery = "inline"in bothconfig.tomland template.desktop-settings.jsonandjq; added tests to verify merge and persistence.Written for commit d0c0f39. Summary will update on new commits.