Repository navigation
Studio: keep tools working after opening a chat file in its app - #14390
NilayYadav wants to merge 17 commits into
Conversation
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. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
for more information, see https://pre-commit.ci
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 908763f324
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| spare = entry / f".{child.name}.copy" | ||
| try: | ||
| shutil.copyfile(child, spare, follow_symlinks = False) | ||
| os.replace(spare, child) |
There was a problem hiding this comment.
Isolate descriptors held by legacy apps before tools run
When a default app keeps the legacy staged file descriptor open across a Studio restart, os.replace detaches only the staged pathname; that descriptor still references the same inode as the sandbox file. The replacement drops the sandbox file's link count to one, so scan_workdir_for_host_channels now permits tools even though the host app can still observe tool writes or overwrite the chat file through its descriptor. The migration needs to break the sandbox-side alias as well, or otherwise avoid re-enabling tools while the old inode remains open outside the sandbox.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. I compared a real run on three versions: the merge base, 5b02fbc, and 7aaeafa. Each run had a chat file notes.csv hard-linked into open-staging/legacy as an older build left it, an fd opened on the staged name and held like an app, then the prune, then a write through that fd. In all three:
- before the prune the scan refused with "hard-linked from outside it";
- after the prune the chat file is at
nlink=1, the scan returns(), the app fd still points at the chat inode, and its write shows up in the chat file (b'X,b\\n').
The only difference is that on 7aaeafa the staged name still exists, as a private copy, so the app can save by name. The merge base drops the same name after 24 h, and this PR's earlier rmtree dropped it at once, with exactly the same open-fd state. So this change does not create the alias and does not widen it.
Breaking the chat-side alias would mean finding and rewriting the matching file inside some chat's workdir from the cache prune. The scan checks names, because a hard link is how a tool could reach a host path. An fd held by the user's own app, on a file the user opened, gives a tool no way out of the sandbox. Run script: repro/legacy_fd_run.py in the review notes.
|
@codex security review |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Review + reproduction evidencePinned review: I ran the real flow on the merge base
Fixes I pushed during review:
Conclusion: the defect is real on main and the head fixes it at the root. I have not shown the refusal itself live: the Linux host I used has no working bubblewrap, and no macOS run was done, so the before/after inside a real OS sandbox is still unproven. |

With the OS sandbox on (High on macOS, or Linux with bubblewrap), using "Open in default app" on a file a tool made, such as chart.png, broke the chat. Every later Python or Terminal call failed with "the session workdir contains a file hard-linked from outside it", because the app was handed a hard link to the chat's file. Nothing removed that link until another Open at least a day later.
The app now gets a private copy instead of a link. Links left by earlier versions are removed when Studio starts, so chats already broken work again after updating. A copy that fails partway no longer leaves a partial file behind.
The app sees a snapshot: open the file again to see a newer version, and edits saved from the app stay in Studio's cache (cleared after a day) rather than in the chat folder. Each Open also writes a full copy, so opening a large video several times keeps several copies on disk until the daily cleanup.