Refactor managed hook runtime and PTY spawn env ownership#1309
Draft
SSwser wants to merge 15 commits intostablyai:mainfrom
Draft
Refactor managed hook runtime and PTY spawn env ownership#1309SSwser wants to merge 15 commits intostablyai:mainfrom
SSwser wants to merge 15 commits intostablyai:mainfrom
Conversation
Move managed hook scripts to the shared appData runtime, route Windows hook launches through a bash wrapper that preserves cmd.exe flags, and keep the daemon dev PATH prepend from dropping inherited Windows PATH entries. Also record the unified runtime + PTY env target architecture in the managed hook runtime spec.
Tasks 8 and 9 (combined since they are tightly coupled): Task 8 - Resolve PTY spawn env in main process: - Add resolvePtySpawnEnv() in pty.ts to centralize env resolution - Call it in pty:spawn handler before provider delegation - Update DaemonPtyAdapter to pass ambientEnv to daemon subprocess - Update all pty.test.ts assertions to use ambientEnv/envOverrides API - Fix preAllocatedHandle allocation for local PTYs - Remove buildSpawnEnv configuration from LocalPtyProvider Task 9 - Remove PTY provider host env repair: - LocalPtyProvider: trust caller's ambientEnv, don't merge process.env - DaemonPtyAdapter/pty-subprocess: same - drop process.env merge - Both still add terminal-shape defaults (TERM/COLORTERM/LANG/etc.)
- SSH spawns now get ambientEnv: { ORCA_TERMINAL_HANDLE: ... }
- Remote agents can self-identify without full host env resolution
- Update SSH spawn test to expect terminal handle injection
- Fixes 'injects ORCA_TERMINAL_HANDLE for non-local PTY providers' test
Contributor
Author
|
Linked issue: #1308 I opened the issue first so the two reproducible user-visible failures and the underlying ownership problem have a stable discussion thread. This Draft PR is the concrete implementation proposal for that direction. |
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
Problem statement
This branch is attached to #1308.
The current behavior exposed two reproducible problems:
While investigating those failures, the deeper issue turned out to be ownership drift across layers:
What changed
Managed hook runtime
userData-derived runtime root, e.g.orca-devvsorca)PTY environment ownership
ambientEnv,envOverrides, andenvToDeleteWhy this is one PR
I realize this is a large change set, so I am opening it as a Draft PR specifically to get maintainer feedback on scope and direction before expecting merge.
I chose to keep the fixes together because the two visible bugs came from the same ownership problem. Splitting only the symptoms from the infrastructure changes would leave the old contracts in place and make the fix rationale harder to review.
If maintainers would prefer this split into follow-up PRs after agreeing on direction, I am happy to do that.
Testing
Feedback requested
I would especially appreciate feedback on:
userDatanamespace matches project expectationsMade with Orca 🐋