fix: isolate CLI installer shell environment - #3217
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe8250d28f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
clawsweeper
Bot
added
merge-risk: 🚨 security-boundary
|
Codex review: needs changes before merge. Reviewed August 26, 2026, 4:59 PM ET / 20:59 UTC. ClawSweeper reviewWhat this changesThe PR switches the repository CLI installer to a protected POSIX shell and clean environment before its administrator-approved symlink command, adds macOS regression coverage, and documents the boundary. Regression provenancePossible regression — probable (reviewed change; known regression link). No predecessor PR is attributed. Merge readinessKeep open: this owner-authored PR is a focused candidate fix for the linked installer privilege-boundary report. The security hardening and regression coverage are sound on source review; remove the release-owned changelog edit and let exact-head macOS checks finish. Priority: P1 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherThe repository CLI installer validates CodexBar’s bundled helper and asks macOS to create two system-wide command symlinks. Its shell environment crosses into an administrator-approved command, so inherited startup state must be excluded before validation and privileged execution. flowchart LR
A[User runs repository installer] --> B[Protected POSIX shell]
B --> C[Validate bundled CLI helper]
C --> D[Clean environment]
D --> E[macOS administrator command]
E --> F[Create CLI symlinks]
F --> G[CodexBar command available]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the protected-shell and clean-environment boundary after exact-head macOS validation, while leaving release notes to the release owner. Do we have a high-confidence way to reproduce the issue? Yes: current-main source explicitly constructs an administrator-boundary Is this the best way to solve the issue? Yes: protected shell startup plus a clean environment before AppleScript removes the reported inherited-function and startup-hook inputs without changing the approval or symlink contract. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against cf79d1310493. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Maintainer disposition of the latest automated review: retaining the changelog entry. This is a maintainer-authored user-visible fix, and our maintainer workflow adds the unreleased note before landing. No version or release publication is being performed here. The request to remove it is not an accepted finding. The reported verification failure occurred during package-manager setup before the macOS installer test executed; it is not an installer regression result. The security proof boundary remains explicit: actual inherited Bash functions and startup hooks are tested, and the generated AppleScript command is captured and exercised against temporary destinations. No administrator authorization request or real system symlink write was used for testing. The administrator approval mechanism and both destination paths are unchanged. |
|
Landed on main as 0b54a92. This fixes #3205 while retaining administrator approval, helper validation and both existing CLI symlink destinations. Verification: Exact-head CI passed at https://github.com/steipete/CodexBar/actions/runs/33012784084: Linux x64/ARM builds/tests/smoke checks, both macOS Swift-test shards, macOS installer/lint checks, provider-engine goldens, final aggregate and GitGuardian. The existing path gate skipped the unrelated musl build. Testing did not request administrator authorization or write real system symlinks. AppleScript returned the generated command, which was exercised only against temporary destinations. The in-app Foundation installer is unchanged. The changelog remains under 0.55.2 Unreleased; no release was published. Thanks @Vectrain51 for the report. |
1 participant
Summary
Closes #3205.
The repository CLI installer previously launched a nested Bash process at the administrator boundary. This change starts the system POSIX shell with startup protection enabled, uses absolute system tools, and clears the inherited environment before starting AppleScript. The administrator approval requirement, helper validation, and both existing symlink destinations remain unchanged. Shell startup protection blocks inherited functions before validation and error handling; it does not elevate privileges.
Adds a focused regression test to local/macOS CI checks and documents the installer boundary. The in-app Foundation-based installer is unchanged.
Verification
/bin/bash Scripts/test_install_codexbar_cli.shpassed: actual macOS Bash-exported functions (including[,set, andexit), startup hooks, inherited shell options, quoting, missing or non-executable helper, fixed destinations, temporary symlink creation, and failure propagation. Positive controls confirm the unprotected system shell imports those functions.make checkpassed.make testpassed: 933 selections across 78 groups, all passing on the first attempt, with no retries or timeouts. No Swift source changed afterward; focused installer checks andmake checkwere rerun for the shell follow-up.be7cdba409ded64be6b678f279f8cc5f636b46c5: Linux x64/ARM builds, tests and CLI smoke checks; both macOS Swift-test shards; installer/macOS lint checks; provider-engine goldens; and final aggregate. The unrelated musl job was skipped by the existing path gate. GitGuardian also passed.No administrator request or real system-destination write was used for testing. AppleScript only returned the generated command; execution tests used temporary destinations. Actual macOS authorization behavior was not exercised.
Thanks @Vectrain51 for the report.