Repository navigation
ENG-1232 install.sh: auto-update on, stale marketplace re-pointed (twin of internal #195) - #290
Conversation
…in of agent-plugins-internal#195) Same file as agent-plugins-internal main after #195. Claude Code's marketplace auto-update is on by default (--no-auto-update to leave it), set in settings.json and known_marketplaces.json after the add; a `memhub` marketplace still pointing at XTraceAI/memhub-claude-plugin is re-pointed before the add instead of aborting under set -e. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🧠 Session context1 session behind this pull request. Team rules that fired while building this
Effort Sessions
|
![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: 228471891e
ℹ️ 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".
| tmp.write_text(json.dumps(data, indent=2) + "\n") | ||
| tmp.replace(path) |
There was a problem hiding this comment.
When settings.json is currently mode 0600 but the user's umask permits group/world reads (commonly 022), write_text() creates the temporary file as 0644 and replace() transfers that mode to the live settings file. The default auto-update path therefore makes previously private Claude settings readable by other local users, potentially exposing values stored under settings such as env; preserve the original file mode when performing this atomic replacement. The earlier re-pointing write has the same issue.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4a41447: both rewrites copy the existing mode onto the temp file before the rename (a file the auto-update step creates gets 0600). Verified in an isolated CLAUDE_CONFIG_DIR under umask 022: a 0600 settings.json stays 0600.
…ex P1) write_text() created the temp file with the umask default and the rename carried that mode onto settings.json, turning a 0600 file into 0644. Both rewrites now copy the existing mode onto the temp file; a file the auto-update step creates gets 0600. Same file as agent-plugins-internal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a414478ff
ℹ️ 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".
| tmp = path.with_name(path.name + ".memhub-tmp") | ||
| tmp.write_text(json.dumps(data, indent=2) + "\n") | ||
| # Keep the mode the file already has (0600 on many machines); a file this | ||
| # step creates gets 0600, since settings can hold secrets. | ||
| os.chmod(tmp, stat.S_IMODE(path.stat().st_mode) if path.exists() else 0o600) |
There was a problem hiding this comment.
When the user's umask permits group/world reads and the Claude config directory is traversable, write_text() writes the complete settings contents—including possible secrets—before the subsequent chmod restricts access; interruption between these calls also leaves the readable .memhub-tmp behind indefinitely. The newly added chmod preserves the final file mode but does not close this exposure window, and the earlier re-pointing write has the same ordering, so create/open the temporary file with its restrictive mode before writing any data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8c370ad: the temp file is opened 0600 (os.open) and only then given the target's mode, and the rewrite resolves settings.json first so a symlink stays one. Verified in an isolated CLAUDE_CONFIG_DIR with a symlinked 0600 settings.json under umask 022.
| # Keep the mode the file already has (0600 on many machines); a file this | ||
| # step creates gets 0600, since settings can hold secrets. | ||
| os.chmod(tmp, stat.S_IMODE(path.stat().st_mode) if path.exists() else 0o600) | ||
| tmp.replace(path) |
There was a problem hiding this comment.
When settings.json is managed as a symlink (for example by a dotfiles manager), tmp.replace(path) replaces the symlink directory entry rather than updating its target. A default installer run that changes autoUpdate therefore silently converts the link into an independent regular file, stops future dotfiles updates from reaching Claude, and leaves the managed target unchanged; resolve the symlink target before performing the atomic replacement or otherwise preserve the link.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8c370ad: the temp file is opened 0600 (os.open) and only then given the target's mode, and the rewrite resolves settings.json first so a symlink stays one. Verified in an isolated CLAUDE_CONFIG_DIR with a symlinked 0600 settings.json under umask 022.
…ed settings.json stays one (Codex P2s) Same file as agent-plugins-internal. The temp file is opened 0600 and takes the target's mode afterwards; the rewrite resolves settings.json first, so a dotfile-managed symlink keeps pointing at its target. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c370adfd1
ℹ️ 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".
| && "$CLAUDE_BIN" plugin update "$PLUGIN@$CLAUDE_MKT" --scope user; } \ | ||
| || say "warning: could not refresh $PLUGIN in Claude Code; re-run this installer, or: claude plugin update $PLUGIN@$CLAUDE_MKT --scope user" >&2 | ||
| "$CLAUDE_BIN" plugin enable "$PLUGIN@$CLAUDE_MKT" >/dev/null 2>&1 || true | ||
| if [ "$AUTO_UPDATE" = 1 ]; then |
There was a problem hiding this comment.
When an existing marketplace has autoUpdate: true and the installer is rerun with --no-auto-update, plugin marketplace add can remove that property—the change itself notes that this command drops the flag on reruns—but this conditional skips the only code that restores it. Consequently, the option advertised as leaving the setting unchanged can silently disable updates; capture and restore the prior value around the add, or avoid the mutating add in this mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ec082a0: the installer records whether autoUpdate was on before marketplace add and restores it when --no-auto-update is given, so the flag leaves it as it was. Verified in an isolated CLAUDE_CONFIG_DIR: on → re-run with --no-auto-update → still on; fresh --no-auto-update → not set.
…lready on (Codex P2) Same file as agent-plugins-internal. The installer records whether autoUpdate was on before `marketplace add` (which drops it on a re-run) and restores it when --no-auto-update is given, so the flag means "leave it as it is". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec082a04ac
ℹ️ 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".
| with os.fdopen(os.open(tmp, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600), "w") as f: | ||
| f.write(json.dumps(settings, indent=2) + "\n") |
There was a problem hiding this comment.
When .memhub-tmp already exists with permissive permissions—for example, after an interrupted run of the earlier implementation—os.open(..., O_CREAT, 0o600) preserves its existing mode, so this writes the complete settings (including possible secrets) before the later chmod. This is fresh evidence beyond the prior fix: the 0o600 argument only applies when creating a new file. Remove/create the temporary file exclusively or set its mode before writing; the flip() rewrite has the same issue.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bb45c93: any leftover .memhub-tmp is unlinked first and the file is opened O_CREAT|O_EXCL 0600, so an interrupted earlier run cannot leave a permissive mode on it. Verified in an isolated CLAUDE_CONFIG_DIR with stale 0644 temp files pre-seeded: settings.json stays 0600, no temp file left.
… app by default Same file as agent-plugins-internal. Runs setup_claude_fork.py install so CLAUDE_CODE_FORK_SUBAGENT=1 lands in settings.json and rule drafts run in the desktop app, the Agent SDK and claude -p, not only the terminal. --no-fork-agent leaves it alone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
…dex P2) Same file as agent-plugins-internal: unlink any leftover temp file first and open with O_EXCL, so an interrupted earlier run cannot leave a permissive mode on the file the settings are written into. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb45c93cfd
ℹ️ 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".
| tmp = real.with_name(real.name + ".memhub-tmp") | ||
| tmp.unlink(missing_ok=True) # a leftover from an interrupted run keeps its old mode | ||
| with os.fdopen(os.open(tmp, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600), "w") as f: |
There was a problem hiding this comment.
When two installers run concurrently against the same Claude config, both use the same .memhub-tmp pathname: one process can unlink the other process's open temporary file, then rename the replacement file while the other is still writing it. This can temporarily expose partial JSON, apply the wrong saved mode or settings snapshot, and causes the displaced process to abort with FileNotFoundError at replace(). The flip() rewrite has the same race; create a unique temporary file per invocation (for example with tempfile.mkstemp) before atomically replacing the target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ae5c936: each rewrite now uses tempfile.mkstemp beside the real file (unique name, 0600 from creation), then chmod to the target's mode and os.replace — so concurrent runs cannot unlink or rename each other's temp file. Verified in an isolated CLAUDE_CONFIG_DIR with a symlinked 0600 settings.json.
Same file as agent-plugins-internal: tempfile.mkstemp beside the real file, unique and 0600 from creation, so concurrent runs cannot clobber each other's temp file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae5c936d78
ℹ️ 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".
| with os.fdopen(fd, "w") as f: | ||
| f.write(json.dumps(data, indent=2) + "\n") | ||
| os.chmod(tmp, mode) | ||
| os.replace(tmp, real) |
There was a problem hiding this comment.
When two installers updating different marketplaces (for example, production and staging) run concurrently, each flip can read the same settings snapshot, change only its own entry, and then replace the file; the last replacement silently drops the other run's autoUpdate change and can likewise overwrite an unrelated concurrent settings edit. The unique temporary filenames remove the earlier pathname collision, but they do not serialize this read-modify-replace sequence, so lock the settings file or re-read and merge while holding a lock before replacement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this one. The installer is a one-shot command a person runs; two of them racing on the same config dir for different marketplaces is not a supported flow, and Claude Code's own plugin marketplace add does the same unlocked read-modify-replace on settings.json immediately before this step, so a lock here would not make the sequence atomic anyway. The rewrite is atomic per file (mkstemp + rename), which is the guarantee that matters for an interrupted run.
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
Linear: ENG-1232 — twin of XTraceAI/agent-plugins-internal#195, which landed the installer change internally (install.sh is not part of the promotion export, so it is ported by hand; a follow-up makes the export carry it).
What changes in
install.sh--no-auto-updateto leave it):autoUpdate: trueon the marketplace'sextraKnownMarketplacesentry in settings.json and inplugins/known_marketplaces.json— the toggle/pluginoffers.marketplace addon a re-run drops the flag from settings, so it is set after the add, every run.memhubmarketplace still points atXTraceAI/memhub-claude-pluginmademarketplace add XTraceAI/agent-pluginsexit 1 underset -e. The declared entry is re-pointed first (the fix the CLI's error names), so the add re-clones from the catalog and keeps the installed plugin and its options.Only Claude Code has a marketplace auto-update setting; Codex and Cursor still rely on a re-run.
The file is byte-identical to internal
main. Verified there end to end in an isolatedCLAUDE_CONFIG_DIR(install, flags set in both files, re-run idempotent).🤖 Generated with Claude Code
Also: the desktop fork agent
9eafca2(public) /6e644fa(internal) — the installer now runssetup_claude_fork.py installby default, soCLAUDE_CODE_FORK_SUBAGENT=1lands in settings.json and rule drafts run in the desktop app, the Agent SDK andclaude -p, not only the terminal.--no-fork-agentleaves it alone. Verified in an isolatedCLAUDE_CONFIG_DIR: set on a fresh install, "already set" on re-run, untouched with the flag.