Repository navigation
🤖 perf: resolve only same-name stored pins when pinning a file - #7918
Merged
Merged
Conversation
…kout File tool cards show "Open as artifact" for every path the agent read, but pinFile refuses absolute paths outside the checkout. This test pins such a file and fails with "Only files inside the workspace checkout can be pinned". --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
File tool cards show "Open as artifact" for every path the agent read, including plans, skill files and scratch files outside the checkout. The backend refused them with "Only files inside the workspace checkout can be pinned", so the button was a dead end. A pin outside the checkout is now stored by its normalized absolute path on the checkout's host (local, worktree, SSH, Docker). It is read from its own folder by the same capped readers, which still refuse a symlinked file, folders and missing files. Checkout files keep their checkout-relative pins. Dev container workspaces still refuse outside files: tool paths there name container files, and pins are read on the host. Older versions drop absolute entries when they read the store. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
… comments A drive-letter path is not absolute on a POSIX host, so it stays a checkout-relative request (refused, as before). Comments that said a pin path is always checkout-relative now say it can be absolute. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
…and drive-letter entries Three failing tests from the review of #7915: a pinned read whose folder is `/` is refused, one file reached through a symlinked folder is two pins, and a hand-edited `C:/x.md` entry on a POSIX host is read below the backend's working folder. Adds resolveRealDirOnRuntime, which the fix uses, with its own test. _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
…m root Follow-up to the review of #7915: - A pin outside the checkout stores the file name in its folder's real path. One file reached through a symlinked folder is now one pin, a symlinked folder into the checkout gives a checkout pin, and SSH and Docker read files in a symlinked folder (their read script refuses a symlinked root). - Containment checks accept `/` as a folder (host reader and the runtime read script), so a file directly under `/` can be pinned. - A stored entry is absolute only by the location's own rules, so a hand-edited `C:/x.md` on a POSIX host is invalid instead of a path below the backend's working folder. _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
_Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
# Conflicts: # src/node/services/pinnedArtifactFiles.test.ts # src/node/services/pinnedArtifactFiles.ts
A hand-edited drive-letter entry on a POSIX host stayed in the pinned list as a pin that never reads. listPinnedFiles now drops entries that are neither absolute on the host nor a valid checkout-relative path. _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
… checkout End-to-end check of the runtime path: pinFile stores the folder's real path and readPinnedFile reads it through the runtime read script. On main's code the pin is refused. _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
…entry An outside pin that #7915 stored through a symlinked folder kept its alias path, and re-pinning it now produced the real path, so the file listed twice. pinFile compares absolute entries by their real form and opens the stored entry instead of adding a second one. _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
Pinning a new file on an SSH checkout runs one folder resolution per stored outside pin (about 2N+2 execs at once). This test pins a new file with three unrelated stored pins and expects none of them resolved. _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
findExistingPin resolved every stored outside pin to find an alias of the new pin, about 2N+2 execs at once on SSH. Only an outside pin's folder is resolved, so an alias keeps its file name: resolve just those entries. Failed resolutions are now logged at debug level. _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
…cost # Conflicts: # src/node/services/pinnedArtifactFiles.test.ts # src/node/services/pinnedArtifactFiles.ts
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. |
Contributor
🟢 e2e stories (Haiku): 14 passed22 agent steps · 7 replayed from cache · 15 model calls · 47.0k tokens (23% cached) All 14 tests in 9 files
e2e 0.20.0 · 1m 20s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 7.9k tokens (22% cached) All 3 tests in 3 files
e2e 0.20.0 · 27.3s · electron · run artifacts 🟢 e2e agent repros (Haiku): no tests selectedSpend: $0.00 of the $1.00 cap (proxy ledger) e2e 0.20.0 · 7.9s · 2 targets (web, phone) · run artifacts |
Member
Author
|
Codex Review: Didn't find any major issues. Keep them coming! 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 Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #7916. When the user pins a new file,
findExistingPinlooks for a stored alias of it. It resolved the folder of every stored outside pin to do that. On an SSH or Docker checkout each resolution is one remote command, and they ran at once: about 2N+2 commands for N stored outside pins, which can hit an SSH server's session limit. Now it resolves only the stored entries with the same file name (usually none), and it logs a failed resolution at debug level instead of dropping the error.Background
The independent review of #7916 found the cost and the silent
.catch(() => null). #7916 had used 5 Codex rounds, so the fix ships here instead of as a sixth push there.Implementation
An outside pin stores its folder's real path plus the file name as given (#7916), so an alias of the same file always has the same file name. Filtering by name keeps every alias match and skips every other entry. A failed resolution still means "no match", and the pin is added as a new entry, as before.
Validation
The first commit adds a failing test. It pins a new file on an SSH checkout (a
LocalRuntimestands in for the host) with three unrelated stored outside pins, and counts the folder resolutions. Before the fix the three stored folders were resolved. After it, none are. The alias test from #7916 still passes.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$15.82