fix: don't cause fatal issues when doing update checks inside of agent sandbox#8186
fix: don't cause fatal issues when doing update checks inside of agent sandbox#8186sean-roberts merged 2 commits intomainfrom
Conversation
📊 Benchmark resultsComparing with cf93d1f
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded filesystem/path/os imports and a new canWriteConfigStore() helper in bin/run.js. The helper resolves the configstore directory (honoring XDG_CONFIG_HOME with a fallback to ~/.config/configstore, walking up to an existing parent) and uses accessSync(..., W_OK) to test writability without throwing. Update-notification (getPackageJson() + updateNotifier(...).notify()) is now executed only when canWriteConfigStore() returns true; when the directory is not writable, update checking and its “Error checking for updates” logging are skipped. Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/run.js`:
- Around line 43-47: The writability check in canWriteConfigStore() incorrectly
calls accessSync on the non-existent target configDir and returns false on
ENOENT; change it to walk up from configDir using path.dirname until you find an
existing ancestor (using fs.existsSync or try statSync) and then call
accessSync(..., constants.W_OK) on that existing ancestor (references:
canWriteConfigStore, accessSync, constants.W_OK, configDir) so the function
returns true when the parent is writable and the configstore directory can be
created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/run.js (1)
41-69: Consider extractingcanWriteConfigStore()into a small utility with unit tests.This logic is now core to startup behavior; isolating it will make edge cases (missing dir, unwritable ancestor, custom
XDG_CONFIG_HOME) easier to lock down with tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/run.js` around lines 41 - 69, Extract the canWriteConfigStore function into a small exported utility (e.g., export function canWriteConfigStore(...)) and replace the inline implementation in bin/run.js with an import and call; add unit tests covering the key scenarios: XDG_CONFIG_HOME set to a custom directory, missing configstore directory where an ancestor is writable vs not (returns false), and unwritable directory/ancestors (accessSync throwing), and ensure tests mock filesystem calls (existsSync, dirname, accessSync, homedir) to assert behavior and error handling; keep the function signature and behavior identical so updateNotifier usage remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/run.js`:
- Around line 41-69: Extract the canWriteConfigStore function into a small
exported utility (e.g., export function canWriteConfigStore(...)) and replace
the inline implementation in bin/run.js with an import and call; add unit tests
covering the key scenarios: XDG_CONFIG_HOME set to a custom directory, missing
configstore directory where an ancestor is writable vs not (returns false), and
unwritable directory/ancestors (accessSync throwing), and ensure tests mock
filesystem calls (existsSync, dirname, accessSync, homedir) to assert behavior
and error handling; keep the function signature and behavior identical so
updateNotifier usage remains unchanged.
🤖 I have created a release *beep* *boop* --- ## [25.2.0](v25.1.1...v25.2.0) (2026-04-22) ### Features * use database as primary name for new db subcommands ([#8176](#8176)) ([40e56cf](40e56cf)) ### Bug Fixes * don't cause fatal issues when doing update checks inside of agent sandbox ([#8186](#8186)) ([acaf478](acaf478)) * filter out intentional user-input errors from error-reporting-lamda ([#8188](#8188)) ([b5b60e3](b5b60e3)) * improve reliability of anon deploys ([#8170](#8170)) ([95b5e52](95b5e52)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: token-generator-app[bot] <82042599+token-generator-app[bot]@users.noreply.github.com>
🎉 Thanks for submitting a pull request! 🎉
Summary
Fix: gracefully handle update check when config store is not writable
When the CLI runs with restricted permissions, update-notifier's internal configstore fails with EPERM and registers a process.on('exit') handler that prints a scary error box — which we can't catch or suppress. It also causes unrecoverable errors in clis like codex. This skips the update check entirely when the config directory isn't writable, avoiding the error without affecting normal usage.
Fixes #<replace_with_issue_number>
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)