fix(windows): stop the ownership guard from reading a closing handle

The guard called GetProcessId(hShell) to prove identity, but the exit
watcher closes hShell on another thread -- so the guard could read a
closed handle, and under strict handle checks that is fatal rather than
merely wrong. Worse, it widened the gap between validating hJob and
using it from two instructions to a kernel round-trip, and handle values
recycle: the likeliest occupant of a freshly recycled value in this
process is another pane's job.

The pid never needed a handle. It is captured at spawn and compared as a
DWORD, so the guard touches no handle at all, and hShell is now closed
inside the same lock as hJob.

Also from review:
- reject CR/LF in a cmd argument. cmd ends the command at a raw line
  break whatever the quote state, so there is no escape for it; encoding
  one anyway truncates the argument and can leave the remainder to run
  as a command. Agent prompts are this encoder's motivating input.
- ask the process table only for the fields a caller needs. Memory and
  CommandLine each cost an OpenProcess per process, inline, for every
  process on the box -- and the 1024 bound is patched out. Ancestry
  reads now skip both.
- corpus gains the degenerate quote-only and two-quote arguments.
- PtyListJobProcessIds' docblock still taught the empty-list contract
  that was corrected on the TS side, and now records that the ConPTY
  console host is never a job member.
- drop a write to NumberOfAssignedProcesses, which is output-only.
- pty_baton::hShell is initialised; ownsShell was only safe because &&
  short-circuited ahead of it.

The backgrounded-child test is rescoped: 'start /b' uses
CREATE_NEW_CONSOLE, not CREATE_BREAKAWAY_FROM_JOB, so it proves job
membership does not block backgrounding -- not that BREAKAWAY_OK works.
That flag rests on the Win32 contract, and I have said so rather than
letting the test imply coverage it does not have.

config/patches/node-pty@1.1.0.patch

@@ -430,7 +430,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..383df0c9c48355547c65e6c9bbba593d
  }
  #endif
 diff --git a/src/win/conpty.cc b/src/win/conpty.cc
-index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb81731730 100644
+index 7b286d3d644c26141df516929703aa6e129df4b2..cad5cbcee5db36f510158e900e21849b298f4de5 100644
 --- a/src/win/conpty.cc
 +++ b/src/win/conpty.cc
 @@ -18,6 +18,7 @@
@@ -441,15 +441,23 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb
  #include <vector>
  #include <Windows.h>
  #include <strsafe.h>
-@@ -46,10 +47,22 @@ struct pty_baton {
- 
-   HANDLE hShell;
+@@ -44,12 +45,29 @@ struct pty_baton {
+   HANDLE hOut;
+   HPCON hpc;
  
+-  HANDLE hShell;
++  HANDLE hShell = nullptr;
++  // Orca: the shell's pid, captured at spawn. The ownership guard compares
++  // against this rather than calling GetProcessId(hShell), because the exit
++  // watcher closes hShell on another thread -- reading it there is an
++  // invalid-handle operation, and under strict handle checks that is fatal.
++  DWORD shellPid = 0;
++
 +  // Orca: job object owning this pty's whole process tree. Null when the OS
 +  // refused to create or assign one (an outer job without breakaway rights),
 +  // in which case callers fall back to their pre-job behaviour.
 +  HANDLE hJob = nullptr;
-+
+ 
    pty_baton(int _id, HANDLE _hIn, HANDLE _hOut, HPCON _hpc) : id(_id), hIn(_hIn), hOut(_hOut), hpc(_hpc) {};
  };
  
@@ -464,16 +472,19 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb
  static volatile LONG ptyCounter;
  
  static pty_baton* get_pty_baton(int id) {
-@@ -103,7 +116,22 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) {
+@@ -102,8 +120,24 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) {
+     // Get process exit code.
      GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code));
      // Clean up handles
-     CloseHandle(baton->hShell);
+-    CloseHandle(baton->hShell);
 -    assert(remove_pty_baton(baton->id));
 +    // Orca: release the job once the shell is gone. Without kill-on-close this
 +    // only frees the handle -- anything the user backgrounded is orphaned, as
 +    // it was before this patch.
 +    {
 +      std::lock_guard<std::mutex> guard(ptyJobMutex);
++      CloseHandle(baton->hShell);
++      baton->hShell = nullptr;
 +      if (baton->hJob != nullptr) {
 +        CloseHandle(baton->hJob);
 +        baton->hJob = nullptr;
@@ -488,7 +499,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb
  
      auto status = tsfn.BlockingCall(exit_event, callback); // In main thread
      switch (status) {
-@@ -416,7 +444,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
+@@ -416,7 +450,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
        nullptr,                      // lpProcessAttributes
        nullptr,                      // lpThreadAttributes
        false,                        // bInheritHandles VERY IMPORTANT that this is false
@@ -500,7 +511,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb
        envArg,                       // lpEnvironment
        mutableCwd.get(),             // lpCurrentDirectory
        &siEx.StartupInfo,            // lpStartupInfo
-@@ -426,6 +457,47 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
+@@ -426,6 +463,47 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
      throw errorWithCode(info, "Cannot create process");
    }
  
@@ -548,10 +559,11 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb
    HANDLE hLibrary = LoadConptyDll(info, useConptyDll);
    bool fLoadedDll = hLibrary != nullptr;
    if (useConptyDll && fLoadedDll)
-@@ -440,6 +512,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
+@@ -440,6 +518,11 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
  
    // Update handle
    handle->hShell = piClient.hProcess;
++  handle->shellPid = piClient.dwProcessId;
 +  // Why here and not earlier: LoadConptyDll above can throw, and a baton that
 +  // carries a job but never reaches SetupExitCallback has nothing left to
 +  // close it.
@@ -559,7 +571,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb
  
    // Close the thread handle to avoid resource leak
    CloseHandle(piClient.hThread);
-@@ -567,6 +643,91 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) {
+@@ -567,6 +650,94 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) {
    return env.Undefined();
  }
  
@@ -572,8 +584,8 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb
 + * process tree. Matching the shell pid makes the id unforgeable.
 + */
 +static bool ownsShell(const pty_baton* handle, DWORD expectedShellPid) {
-+  return handle != nullptr && handle->hJob != nullptr && handle->hShell != nullptr &&
-+         expectedShellPid != 0 && GetProcessId(handle->hShell) == expectedShellPid;
++  return handle != nullptr && handle->hJob != nullptr && expectedShellPid != 0 &&
++         handle->shellPid == expectedShellPid;
 +}
 +
 +/**
@@ -604,10 +616,14 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb
 +/**
 + * Orca: the pids still alive in this pty's tree, straight from the kernel.
 + *
-+ * This is the liveness answer a stale JS map cannot give: a pane whose shell
-+ * has exited reports an empty list, so a registry entry can no longer claim a
-+ * terminal is connected when nothing is running.
-+ * Returns null when no job was assigned.
++ * Descendant liveness for a tree that is still tracked, including children that
++ * detached from the console. Once the shell exits the baton is gone, so this
++ * returns null rather than an empty list -- null means "no answer", never
++ * "they died". Also returns null when no job was assigned.
++ *
++ * Does not include the ConPTY console host: CreatePseudoConsole spawns it
++ * before this job exists, so it is not a member and ClosePseudoConsole is what
++ * reaps it.
 + */
 +static Napi::Value PtyListJobProcessIds(const Napi::CallbackInfo& info) {
 +  Napi::Env env(info.Env());
@@ -632,7 +648,6 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb
 +    const size_t bytes = sizeof(JOBOBJECT_BASIC_PROCESS_ID_LIST) + sizeof(ULONG_PTR) * capacity;
 +    std::vector<char> buffer(bytes, 0);
 +    auto* list = reinterpret_cast<JOBOBJECT_BASIC_PROCESS_ID_LIST*>(buffer.data());
-+    list->NumberOfAssignedProcesses = capacity;
 +    if (QueryInformationJobObject(handle->hJob, JobObjectBasicProcessIdList, list, static_cast<DWORD>(bytes), nullptr)) {
 +      auto pids = Napi::Array::New(env, list->NumberOfProcessIdsInList);
 +      for (DWORD i = 0; i < list->NumberOfProcessIdsInList; i++) {
@@ -651,7 +666,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb
  /**
  * Init
  */
-@@ -577,6 +738,8 @@ Napi::Object init(Napi::Env env, Napi::Object exports) {
+@@ -577,6 +748,8 @@ Napi::Object init(Napi::Env env, Napi::Object exports) {
    exports.Set("resize", Napi::Function::New(env, PtyResize));
    exports.Set("clear", Napi::Function::New(env, PtyClear));
    exports.Set("kill", Napi::Function::New(env, PtyKill));

pnpm-lock.yaml

@@ -27,7 +27,7 @@ patchedDependencies:
     hash: 7333b3837f80a7fbd045964db6d76ba4fc118e49134bdbabb00585b6b7b60673
     path: config/patches/lint-staged@16.4.0.patch
   node-pty@1.1.0:
-    hash: a8de16410ba90ef37d6486bebf03aaebcf2963514fbf1f5bd81f56eea7d96939
+    hash: c48841645e1873768dfdfec82cc018f54dec09c42293491aba043650801c09fe
     path: config/patches/node-pty@1.1.0.patch
 
 importers:
@@ -69,7 +69,7 @@ importers:
         version: 3.3.1
       node-pty:
         specifier: ^1.1.0
-        version: 1.1.0(patch_hash=a8de16410ba90ef37d6486bebf03aaebcf2963514fbf1f5bd81f56eea7d96939)
+        version: 1.1.0(patch_hash=c48841645e1873768dfdfec82cc018f54dec09c42293491aba043650801c09fe)
       posthog-node:
         specifier: ^5.33.3
         version: 5.33.3
@@ -12046,7 +12046,7 @@ snapshots:
 
   node-int64@0.4.0: {}
 
-  node-pty@1.1.0(patch_hash=a8de16410ba90ef37d6486bebf03aaebcf2963514fbf1f5bd81f56eea7d96939):
+  node-pty@1.1.0(patch_hash=c48841645e1873768dfdfec82cc018f54dec09c42293491aba043650801c09fe):
     dependencies:
       node-addon-api: 7.1.1
 

src/main/windows/windows-process-table.ts

@@ -24,6 +24,11 @@ import { createProcessTableSnapshotReader } from '../../shared/process-table-sna
  *   PowerShell CIM        706 / 723  ms
  */
 
+/** Which of the per-process lookups a caller actually needs. */
+export type WindowsProcessFields = { memory?: boolean; commandLine?: boolean }
+
+const ALL_FIELDS: WindowsProcessFields = { memory: true, commandLine: true }
+
 export type WindowsProcessRow = {
   pid: number
   ppid: number
@@ -91,7 +96,7 @@ function loadWindowsProcessTree(): WindowsProcessTreeModule | null {
  */
 const WINDOWS_PROCESS_QUERY_TIMEOUT_MS = 3_000
 
-function readNativeRows(): Promise<WindowsProcessRow[]> {
+function readNativeRows(fields: WindowsProcessFields): Promise<WindowsProcessRow[]> {
   const native = moduleLoader()
   if (!native) {
     // Reject rather than resolve empty: an empty table is a claim that nothing
@@ -99,7 +104,13 @@ function readNativeRows(): Promise<WindowsProcessRow[]> {
     // tree dead. "Unavailable" has to stay distinguishable from "empty".
     return Promise.reject(new Error('windows process table unavailable'))
   }
-  const flags = native.ProcessDataFlag.Memory | native.ProcessDataFlag.CommandLine
+  // Why ask for the minimum: both extra flags do an OpenProcess per process --
+  // Memory adds GetProcessMemoryInfo, CommandLine adds a PEB read -- inline,
+  // for every process on the box, and the 1024-entry bound is patched out.
+  // Measured at 1050 processes: 15.9ms p50 for pid/ppid/name, 30.6ms with both.
+  const flags =
+    (fields.memory ? native.ProcessDataFlag.Memory : 0) |
+    (fields.commandLine ? native.ProcessDataFlag.CommandLine : 0)
   return new Promise((resolve, reject) => {
     try {
       const deadline = setTimeout(
@@ -143,8 +154,12 @@ function readNativeRows(): Promise<WindowsProcessRow[]> {
 // Why still cache: the snapshot is cheap but not free, and a worktree delete
 // tears down PTYs 32-wide. The shared TTL + single-in-flight reader collapses
 // that burst into one scan, exactly as the PowerShell path had to.
+// Why one reader and not one per field set: the cache exists so a 32-wide
+// worktree delete collapses into a single scan, and splitting it per caller
+// would restore the fan-out it prevents. The shared snapshot therefore carries
+// every field; the narrow read below is for callers that bypass the cache.
 const snapshotReader = createProcessTableSnapshotReader<WindowsProcessRow[]>({
-  runPs: readNativeRows,
+  runPs: () => readNativeRows(ALL_FIELDS),
   now: () => Date.now()
 })
 
@@ -163,6 +178,16 @@ export function readWindowsProcessTableFresh(): Promise<WindowsProcessRow[]> {
   return snapshotReader.getFreshSnapshot()
 }
 
+/**
+ * An uncached snapshot carrying only pid, ppid and name.
+ *
+ * For teardown identity, which needs ancestry and nothing else: skipping the
+ * two per-process OpenProcess lookups roughly halves the scan.
+ */
+export function readWindowsProcessAncestry(): Promise<WindowsProcessRow[]> {
+  return readNativeRows({})
+}
+
 /** Whether the native table can be read at all on this host. */
 export function isWindowsProcessTableAvailable(): boolean {
   return moduleLoader() !== null

src/main/windows/windows-pty-job.win32.test.ts

@@ -133,11 +133,14 @@ describeOnWindows('ConPTY job ownership', () => {
     }
   }, 60_000)
 
-  it('still lets a child break away from the job', async () => {
-    // A job with no limits denies CREATE_BREAKAWAY_FROM_JOB outright, and
-    // installers, msiexec and some updater paths spawn that way deliberately.
-    // They would fail ONLY inside an Orca terminal, which is the worst shape a
-    // bug report can take, so JOB_OBJECT_LIMIT_BREAKAWAY_OK is load-bearing.
+  it('still starts a backgrounded child from inside the job', async () => {
+    // Scope note: `start /b` uses CREATE_NEW_CONSOLE, not
+    // CREATE_BREAKAWAY_FROM_JOB, so this does NOT prove the BREAKAWAY_OK flag
+    // is doing its job -- it proves job membership does not block ordinary
+    // backgrounding. The flag itself rests on the Win32 contract (a job lacking
+    // JOB_OBJECT_LIMIT_BREAKAWAY_OK denies breakaway with ERROR_ACCESS_DENIED
+    // regardless of its other limits) and is not covered by a test here.
+    // Covering it needs a helper that passes the flag to CreateProcess.
     const nodePty = await import('node-pty')
     const marker = join(mkdtempSync(join(tmpdir(), 'orca-breakaway-')), 'marker.txt')
     const proc = nodePty.spawn('cmd.exe', [], {