fix(core): use unique probe filename in check_credentials_directory_permissions to avoid concurrent race

[ameet-rajababa]

Description

core/utils.check_credentials_directory_permissions() writes a probe file at the fixed path {credentials_dir}/.permission_test to test write+remove permission. When two or more processes share a credentials directory (WORKSPACE_MCP_CREDENTIALS_DIR), they collide on that literal filename and one observes ENOENT during the write/remove sequence. The OSError is caught and re-raised as PermissionError("Cannot write to existing credentials directory ..."), the subprocess exits, and the MCP client sees Connection closed.

The interleaving:

P1 open(.permission_test, "w")    -> ok
P1 write / os.remove              -> ok
P2 open(.permission_test, "w")    -> ok
P2 os.remove                      -> ENOENT (P1 already removed it; or vice versa)
P2 catches (PermissionError, OSError) -> re-raises as PermissionError
P2 exits                          -> MCP client: "Connection closed"

Empirically, in a deployment where six launchd services spawn workspace-mcp subprocesses against the same WORKSPACE_MCP_CREDENTIALS_DIR, this produced ~109 [ERROR] log entries over an 8-day window, correlating ~1:1 with downstream tool-call failures.

Fix

Replace the fixed-name probe with tempfile.mkstemp(prefix=".permission_test_", dir=credentials_dir), which gives each call a unique random suffix — concurrent callers never collide. Extracted into a private helper _probe_credentials_directory_writable() to deduplicate the two probe sites (existing-dir branch and newly-created-dir branch).

mkstemp is preferred over tempfile.NamedTemporaryFile because the latter holds the file open with O_TEMPORARY on Windows for Python ≤3.11, which can fail to delete cleanly when antivirus or indexing services hold a transient handle on the file. Since requires-python = ">=3.10" is the declared floor, NamedTemporaryFile(delete=True) would be a regression risk on Windows.

Existing PermissionError / OSError exception wrapping and messages are preserved.

Reproducer

A multiprocessing regression test is included (tests/test_credentials_dir_permissions.py::test_concurrent_callers_do_not_race_on_probe_filename): 8 worker processes × 50 iterations each, all calling check_credentials_directory_permissions() against the same directory. Fails on main at 0d1475c, passes after this patch.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

Testing

• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes (pytest tests/test_credentials_dir_permissions.py tests/test_main_permissions_tier.py — 13 passed)
• I have tested this change manually (8-worker concurrent test fails on pre-fix main, passes after patch; ruff check clean)

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have enabled "Allow edits from maintainers" for this pull request

Additional Notes

• The new tests don't use @pytest.mark.asyncio, which is correct given they exercise blocking I/O — flagging here pre-emptively given the repo's asyncio_mode = "strict" config.
• Pre-existing tests in tests/test_main_permissions_tier.py that monkeypatch check_credentials_directory_permissions continue to pass — the function's public signature and exception types are unchanged.
• I grepped the rest of the tree for the same probe pattern; this is the only fixed-filename probe-write site.

Summary by CodeRabbit

• Bug Fixes

  • Improved credentials-directory writability check using a safer temporary-file probe, avoiding leftover probe files and reducing race conditions.

• Tests

  • Added tests for directory permission checks, automatic creation, cleanup assurance, read-only error handling, and concurrent access stability.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3b077e91-e92e-4c11-ab12-75059aab3018

📥 Commits

Reviewing files that changed from the base of the PR and between e207742 and 40db1fe.

📒 Files selected for processing (1)

• tests/test_credentials_dir_permissions.py

---

📝 Walkthrough

Walkthrough

Replaced fixed-path credential-directory write tests with a tempfile-based probe (_probe_credentials_directory_writable) and integrated it into check_credentials_directory_permissions(). Added pytest coverage validating creation, cleanup, permission failures, and a multiprocessing concurrency regression test.

Changes

Credentials Directory Writability Probe

Layer / File(s)	Summary
Tempfile-based writability probe core/utils.py	Import tempfile and add _probe_credentials_directory_writable() that creates a uniquely-named temp file via tempfile.mkstemp, writes a sentinel payload, and best-effort cleans up.
Integration into permission checker core/utils.py	Replace the previous fixed-path write/delete checks in check_credentials_directory_permissions() with calls to _probe_credentials_directory_writable() for both existing and newly created directories.
Permission checking test suite tests/test_credentials_dir_permissions.py	Add tests for existing writable directory, automatic creation of missing directories, cleanup verification (no leftover probe files), read-only directory raising PermissionError, and a multiprocessing concurrency regression test to ensure no probe-filename races.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped in with a tempfile name,
a little probe that plays no game.
No stray files left upon the lawn,
concurrent friends all greet the dawn.
Clean and quiet—checks pass on.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: replacing a fixed probe filename with a unique one to avoid race conditions in concurrent access to the credentials directory.
Description check	✅ Passed	The PR description comprehensively covers all template sections: clear problem description with interleaving diagram, detailed fix explanation, testing evidence, and completed checklist items.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_credentials_dir_permissions.py`:
- Around line 58-96: The test docstring for
test_concurrent_callers_do_not_race_on_probe_filename incorrectly states the fix
uses tempfile.NamedTemporaryFile; update the docstring to reflect the actual
implementation in core/utils.py which uses tempfile.mkstemp in
_probe_credentials_directory_writable (i.e., mention mkstemp or “creating a
unique probe file via mkstemp” instead of NamedTemporaryFile) so the docstring
matches the code.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: dfe48d86-23cb-4cc2-a288-dd5788b0f325

📥 Commits

Reviewing files that changed from the base of the PR and between 0d1475c and e207742.

📒 Files selected for processing (2)

• core/utils.py
• tests/test_credentials_dir_permissions.py

[ameet-rajababa]

@taylorwilsdon — friendly nudge on this one when you get a chance. All bot checks are green and CodeRabbit's one nit (docstring/impl mismatch) was addressed in 40db1fe. Happy to rebase if anything has drifted on main.

Thanks!