Repository navigation
fix: address remote command feedback and persistence failures - #78
Conversation
anikitenko
commented
Oct 18, 2026
anikitenko
commented
- Add host adapters for SSH status, live output, and cancellation
- Validate combined output limits and sanitize service errors
- Reject JSON flush failures and support recovery
- Document FDO wiring, timeout semantics, and save acknowledgements
- Add host adapters for SSH status, live output, and cancellation - Validate combined output limits and sanitize service errors - Reject JSON flush failures and support recovery - Document FDO wiring, timeout semantics, and save acknowledgements
Coverage Report
|
![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: 8203de1f86
ℹ️ 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".
| return execResult(value, payload.maxOutputBytes); | ||
| }, error => { throw safeError(error); }); | ||
| void result.catch(() => {}); | ||
| const iterator = iteratorFor(host.events); |
There was a problem hiding this comment.
If startSSHExecution() successfully starts a remote command but returns a malformed events value, iteratorFor(host.events) throws before the cleanup callback is registered. The caller receives no execution handle, while host.cancel() is never invoked, so the remote command can continue running without any way for the plugin to stop it. Establish best-effort cancellation before validating the event source, or catch validation failures and cancel the newly started execution.
Useful? React with 👍 / 👎.
| const result = Promise.resolve(host.result).then(value => { | ||
| assertCurrent(transport, "ssh.exec"); | ||
| return execResult(value, payload.maxOutputBytes); | ||
| }, error => { throw safeError(error); }); |
There was a problem hiding this comment.
When host services are cleared or replaced while an execution is pending, cleanup can cause the old host's result promise to reject, commonly with CANCELLED. Unlike the fulfillment branch, this rejection branch never calls assertCurrent, so the stale old-transport error is exposed as a valid terminal result instead of UNAVAILABLE. Check that the transport is still current before mapping the provider rejection.
Useful? React with 👍 / 👎.
| } finally { closeIterator(iterator); } | ||
| }, | ||
| }; | ||
| void result.finally(() => { activeResources.delete(cleanup); }).catch(() => {}); |
There was a problem hiding this comment.
A result is explicitly allowed to settle before its bounded output events are drained, but settling it immediately removes this execution from activeResources. If host services are then cleared or replaced before the consumer finishes—or when there is no consumer—the teardown no longer calls iterator.return(), leaving the old output subscription and its buffered host resources alive. Keep the resource registered until the event iterator has also completed or been closed.
Useful? React with 👍 / 👎.
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. |
1 participant
