Repository navigation
Conversation
binaricat
added
the
automation:codex-loop
|
@codex review |
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. Chef's kiss. 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". |
netcatty-bot
added
automation:bot-pr
|
Codex reported no major issues. This PR is marked ready for human review/merge. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2df507fda1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Already-running attempts keep the leases they acquired. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [getActivePane, getPaneByConnectionId, getTabByConnectionId, sftpSessionsRef, updateTab], | ||
| [getActivePane, getPaneByConnectionId, getTabByConnectionId, sftpSessionsRef, updateTab, acquireTransferSession], |
There was a problem hiding this comment.
When a saved host is edited while an existing pane for the same host ID remains connected, this dependency makes the next startTransfer capture an acquisition callback that resolves the ID against the updated hosts; meanwhile, the pane's browse session and connection cache key still represent the old endpoint. Because processTransferBody passes only the host ID despite AcquireTransferSessionFn supporting the connect-time host, the UI can list the old server while a download or upload reads from or writes to the newly configured server, potentially overwriting the wrong file. Pass each pane's connect-time host to acquisition, or explicitly reconnect/close old panes, rather than switching every pane to the latest vault endpoint.
Useful? React with 👍 / 👎.
binaricat
added
automation:codex-loop
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b56871450
ℹ️ 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".
| const sourceHost = resolveConnectedHost?.(sourcePane.id); | ||
| const targetHost = resolveConnectedHost?.(targetPane.id); |
There was a problem hiding this comment.
When a remote-file preflight is pending, the user can reconnect either captured pane to a different endpoint under the same host ID before processTransferBody runs. These lookups then read the new mutable tab mapping even though startTransfer still uses the old pane snapshot, paths, and file selection, so the transfer can read from or overwrite the newly connected server. Capture both connect-time hosts alongside the panes before the first preflight await and carry those snapshots into processing.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cda594263b
ℹ️ 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".
| source: panes[0] ? toConnectHost(resolveConnectedHost?.(panes[0].id)) : undefined, | ||
| target: panes[1] ? toConnectHost(resolveConnectedHost?.(panes[1].id)) : undefined, |
There was a problem hiding this comment.
When reconnecting an occupied tab to endpoint B, connect() updates connectedHostByTabIdRef before awaiting closure of endpoint A and before replacing the pane's connection. A transfer started during that await therefore supplies the still-visible pane snapshot and paths for A here, but captures B from the tab-id map; its dedicated leases can then read from or write to B using A's selection. Capture endpoint metadata keyed by the pane's connection ID, or update the tab mapping atomically with the pane connection.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 269a79683a
ℹ️ 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".
| const sourceHostId = task.sourceHostId || sourcePane.connection?.hostId; | ||
| if (acquireTransferSession && sourceHostId) { | ||
| sourceWorkLease = await acquireTransferSession(sourceHostId, `${task.id}:work-source`); | ||
| sourceWorkLease = await acquireTransferSession(sourceHostId, `${task.id}:work-source`, connectHosts.source); |
There was a problem hiding this comment.
When an open pane connects through a saved jump host and that jump host is edited, this passes only the pane's top-level Host snapshot. The refreshed acquisition callback still resolves host.hostChain.hostIds against the current hosts array in buildSftpHostCredentials, so its work and file leases use the edited bastion while the pane and selected paths belong to the old chain. With private hostnames that resolve differently behind each bastion, a download or upload can reach the wrong server; capture the resolved jump-host chain or full connect-time connection options along with the top-level host.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c98bd62dae
ℹ️ 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".
| sameHost, | ||
| 0, false, undefined, undefined, connectHosts, |
There was a problem hiding this comment.
When two panes for the same saved host were connected before and after a jump-host edit, their top-level connection cache keys remain equal because buildCacheKey omits the jump chain, so sameHost is still true even though connectHosts now pins different servers. For legacy-SCP transfers (resumable === false), the same-host path executes cp through the source session and ignores the target SFTP session, causing the transfer to report success while writing the target path on the source-side server rather than the destination. Include the pinned chain identity in the same-host predicate, or disable the optimization whenever the two captured chains differ.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c40232556
ℹ️ 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".
| const chainIdentity = (chain: Host[] | undefined): string => | ||
| (chain ?? []).map(h => `${h.id}@${h.hostname}:${h.port ?? ""}:${h.username}`).join("→"); |
There was a problem hiding this comment.
When the first jump host keeps the same ID, hostname, port, and username but its proxyConfig is edited, the old and new pinned chains serialize identically here even though buildSftpHostCredentials forwards the different proxy and the routes can reach different servers. sameHost consequently remains true, so a legacy-SCP transfer may execute cp only on the source-side server instead of copying to the target endpoint. Fresh evidence in the current code is that this identity omits proxyConfig and other route-affecting fields that the credential builder includes; compare the full resolved jump transport identity or conservatively disable the optimization.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c72fde439e
ℹ️ 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".
| const sameHostPinnedChains = chainIdentity(connectHosts.sourceChain) === chainIdentity(connectHosts.targetChain); | ||
| const sameHost = sameHostEndpoints && sameHostPinnedChains && !!sourceSftpId && !!targetSftpId; |
There was a problem hiding this comment.
When two panes for the same saved host were connected before and after its top-level proxyConfig was edited, their cache keys and empty jump-chain identities remain equal even though the pinned connectHosts route through different proxies. For a legacy-SCP remote-to-remote transfer, electron/bridges/transferBridge.cjs then honors sameHost by running cp only through the source session, so it can report success while writing on the source-side server instead of the target reached through the newer proxy. Compare the pinned top-level proxy identities as well, or disable this optimization whenever either routed endpoint differs.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
netcatty-bot
added
automation:codex-clean
|
Codex reported no major issues. This PR is marked ready for human review/merge. |
Editing a saved SSH host could leave dual-pane SFTP transfers using an earlier endpoint even after opening a fresh pane. Transfers now refresh their acquisition callback when saved hosts change, and use each pane's existing connect-time Host snapshot for both preparation and file leases. A fresh pane uses the edited host; an already-open pane continues transferring to the endpoint it is browsing. Recursive directory children receive the same captured endpoints.
Native Linux Electron reproduction used two real local OpenSSH servers with generated test credentials. Before the fix, a fresh pane could browse the edited host while transfers failed against the old host. Testing the initial callback-only fix also exposed the opposite risk: retaining the old pane after editing the saved host uploaded to the new server, despite browsing the old one. The final change addresses both cases without changing already-acquired leases or adding a new connection registry.
Validation:
Previously validated revision:
5b5687145037e664248de8611ef8a4a7fc277d9f. The earlier2df507fcallback-only revision was held after native review found the retained-pane issue; its CI result does not validate this final revision. Exact-head CI completed successfully: lint/test/build and Windows native broadcast SSH. This PR remains a draft for review; no merge authorization.Current head
cda594263b3a70377e24247d3978ebfe4edf753badditionally captures the same connect-time Host snapshot before preflight awaits and passes it through processing. Fresh native Linux verification at this exact head passed four retained-pane/fresh-pane upload/download cases: old pane continues on container22223, new pane uses edited host22222, distinct downloaded markers and hashes match, and each uploaded file is absent from the other endpoint. Focused tests 12/12 and production build pass. Exact-head CI run38852846945 completed successfully for Linux lint/test/build and Windows native SSH broadcast; earlier CI is historical. Remains draft, no merge authorization.