From bb59a8374593480380ee551f1c50f4fc881ea4ec Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Fri, 9 Oct 2026 13:50:01 -0400 Subject: [PATCH 1/2] fix(skills): set per-skill directory mode to 0755 through the pinned descriptor mkdir's mode argument is masked by the process umask, so under umask 0077 // ended up 0700 and a separate agent identity could not read SKILL.md, though the reconcile reported success. The README promises 0755 regardless of the umask. After a successful create, chmod the directory through the descriptor the code already pins, never its path. A directory that already existed keeps its mode, and the managed root is left to the user. Co-Authored-By: Claude Opus 5.5 --- .../src/launchdarkly_ai_server/safe_fs.py | 22 +++++++++++++++++-- packages/client/tests/test_safe_fs.py | 14 ++++++++++++ packages/client/tests/test_skills_fs.py | 16 ++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/packages/client/src/launchdarkly_ai_server/safe_fs.py b/packages/client/src/launchdarkly_ai_server/safe_fs.py index 14a01661..d6d7353d 100644 --- a/packages/client/src/launchdarkly_ai_server/safe_fs.py +++ b/packages/client/src/launchdarkly_ai_server/safe_fs.py @@ -30,6 +30,11 @@ """Mode set explicitly on every written file: never from the umask, never executable.""" +_DIR_MODE = 0o755 +"""Mode set explicitly on every directory this module creates, because +``mkdir``'s mode argument is masked by the umask: under ``0077`` a separate agent +identity could not traverse the directory to read what is inside.""" + _SUPPORTS_FCHMOD = hasattr(os, "fchmod") """Whether the mode can be set on the descriptor. Probed because Windows only has ``os.fchmod`` from CPython 3.13, and this package supports 3.12.""" @@ -134,8 +139,10 @@ def open_or_create_directory( # Without the *at() family the mkdir below would raise on a dir_fd. if not SUPPORTS_DIR_FD: dir_fd = None + created = False try: - os.mkdir(_at(directory, dir_fd), 0o755, dir_fd=dir_fd) + os.mkdir(_at(directory, dir_fd), _DIR_MODE, dir_fd=dir_fd) + created = True except FileExistsError: # os.stat(follow_symlinks=False) is the spelling os.supports_dir_fd # advertises; it is equivalent to os.lstat. @@ -147,7 +154,18 @@ def open_or_create_directory( raise ValueError("the directory is a symlink") from None if not stat.S_ISDIR(mode): raise ValueError("the path is not a directory") from None - return open_directory_nofollow(directory, dir_fd=dir_fd) + fd = open_directory_nofollow(directory, dir_fd=dir_fd) + # Only a directory this call created: an existing one keeps the mode its + # owner gave it. fchmod on the pinned descriptor, never chmod on the path, + # which a swap between mkdir and open could redirect. Without a descriptor + # (no *at() family, i.e. Windows) there are no POSIX modes to correct. + if created and fd is not None and _SUPPORTS_FCHMOD: + try: + os.fchmod(fd, _DIR_MODE) + except BaseException: + os.close(fd) + raise + return fd @contextmanager diff --git a/packages/client/tests/test_safe_fs.py b/packages/client/tests/test_safe_fs.py index 39842185..83f63765 100644 --- a/packages/client/tests/test_safe_fs.py +++ b/packages/client/tests/test_safe_fs.py @@ -64,6 +64,20 @@ def test_create_makes_the_directory(self, tmp_path: Path) -> None: if fd is not None: os.close(fd) + @pytest.mark.skipif(os.name == "nt", reason="POSIX modes") + def test_create_leaves_an_existing_directory_mode_alone( + self, tmp_path: Path + ) -> None: + """The explicit ``0755`` is for directories this call creates; one that + was already there keeps the mode its owner chose.""" + target = tmp_path / "existing" + target.mkdir() + target.chmod(0o700) + fd = open_or_create_directory(target) + if fd is not None: + os.close(fd) + assert stat.S_IMODE(target.stat().st_mode) == 0o700 + def test_create_refuses_an_existing_symlink(self, tmp_path: Path) -> None: """``Path.mkdir(exist_ok=True)`` would accept this and reopen the hole. diff --git a/packages/client/tests/test_skills_fs.py b/packages/client/tests/test_skills_fs.py index 1ecdeefc..342c0cbf 100644 --- a/packages/client/tests/test_skills_fs.py +++ b/packages/client/tests/test_skills_fs.py @@ -1446,6 +1446,22 @@ async def test_written_file_is_0644_and_not_executable(self, root: Path) -> None assert not mode & stat.S_IXGRP assert not mode & stat.S_IXOTH + @pytest.mark.skipif(os.name == "nt", reason="POSIX modes") + async def test_created_skill_directory_is_0755_under_a_restrictive_umask( + self, root: Path + ) -> None: + """``mkdir``'s mode is masked by the umask, so without an explicit + ``fchmod`` a ``0077`` umask leaves ``//`` at ``0700`` and a + separate agent identity cannot read the ``SKILL.md`` inside it.""" + previous = os.umask(0o077) + try: + report = await write_skills([_skill("a")], root) + finally: + os.umask(previous) + assert report.ok is True, _error_messages(report) + assert stat.S_IMODE((root / "a").stat().st_mode) == 0o755 + assert stat.S_IMODE((root / "a" / "SKILL.md").stat().st_mode) == 0o644 + async def test_write_goes_through_a_single_atomic_rename( self, root: Path, monkeypatch: pytest.MonkeyPatch ) -> None: From 93733e082aa1b9c45ce9ff5f7d0a0d44d69c4b39 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Fri, 9 Oct 2026 14:58:14 -0400 Subject: [PATCH 2/2] fix(skills): keep an inherited setgid bit when setting the directory mode Linux copies a setgid parent's bit onto a new directory so files created inside inherit the shared group. fchmod sets exactly the bits it is given, so the bare 0755 cleared it. Carry S_ISGID over from fstat on the same descriptor. Co-Authored-By: Claude Opus 5.5 --- packages/client/README.md | 3 ++- .../src/launchdarkly_ai_server/safe_fs.py | 8 +++++-- packages/client/tests/test_skills_fs.py | 21 +++++++++++++++++++ 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index 2836d07f..73ba687d 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -837,7 +837,8 @@ however its transport does. **Run `write_skills` as a different identity than the agent.** Reconcile as one user, run the agent as another. The reconcile sets modes explicitly rather than from your umask: skill files and the manifest at `0644` (via `fchmod` on the descriptor, so it cannot be redirected), -per-skill `//` directories at `0755`, and never the execute bit. Those modes only +per-skill `//` directories at `0755` (keeping a setgid bit inherited from a setgid +root, so a shared group still propagates), and never the execute bit. Those modes only protect anything if the two identities differ. **What to verify, as the identity that will run the agent.** The SDK cannot check this for you diff --git a/packages/client/src/launchdarkly_ai_server/safe_fs.py b/packages/client/src/launchdarkly_ai_server/safe_fs.py index d6d7353d..08ba7fed 100644 --- a/packages/client/src/launchdarkly_ai_server/safe_fs.py +++ b/packages/client/src/launchdarkly_ai_server/safe_fs.py @@ -31,7 +31,7 @@ executable.""" _DIR_MODE = 0o755 -"""Mode set explicitly on every directory this module creates, because +"""Permission bits set explicitly on every directory this module creates, because ``mkdir``'s mode argument is masked by the umask: under ``0077`` a separate agent identity could not traverse the directory to read what is inside.""" @@ -161,7 +161,11 @@ def open_or_create_directory( # (no *at() family, i.e. Windows) there are no POSIX modes to correct. if created and fd is not None and _SUPPORTS_FCHMOD: try: - os.fchmod(fd, _DIR_MODE) + # Keep the setgid bit Linux copies from a setgid parent: fchmod sets + # exactly the bits given, so a bare 0755 would clear it and files + # written inside would stop inheriting the shared group. + inherited = os.fstat(fd).st_mode & stat.S_ISGID + os.fchmod(fd, inherited | _DIR_MODE) except BaseException: os.close(fd) raise diff --git a/packages/client/tests/test_skills_fs.py b/packages/client/tests/test_skills_fs.py index 342c0cbf..6eeb08a2 100644 --- a/packages/client/tests/test_skills_fs.py +++ b/packages/client/tests/test_skills_fs.py @@ -1462,6 +1462,27 @@ async def test_created_skill_directory_is_0755_under_a_restrictive_umask( assert stat.S_IMODE((root / "a").stat().st_mode) == 0o755 assert stat.S_IMODE((root / "a" / "SKILL.md").stat().st_mode) == 0o644 + async def test_created_skill_directory_keeps_an_inherited_setgid_bit( + self, root: Path + ) -> None: + """Linux copies a setgid parent's bit onto a new directory so files + inside inherit the shared group. The explicit ``0755`` must not clear + it; ``fchmod`` sets exactly the bits it is given.""" + os.chmod(root, 0o2755) + probe = root / "probe" + probe.mkdir() + inherits = bool(probe.stat().st_mode & stat.S_ISGID) + probe.rmdir() + if not inherits: + pytest.skip("new directories do not inherit setgid on this platform") + previous = os.umask(0o077) + try: + report = await write_skills([_skill("a")], root) + finally: + os.umask(previous) + assert report.ok is True, _error_messages(report) + assert stat.S_IMODE((root / "a").stat().st_mode) == 0o2755 + async def test_write_goes_through_a_single_atomic_rename( self, root: Path, monkeypatch: pytest.MonkeyPatch ) -> None: