feat(win32): default-terminal registration via ITerminalHandoff3 (#130) - #188
Conversation
|
Warning Review limit reachedNext included review available in 53 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (40)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
7fdc981 to
4f1d031
Compare
|
@coderabbitai review Context for reviewers: Registers noctty as a Windows default-terminal provider: a classic out-of-process COM server implementing Prior adversarial review audited every GUID/IID on the branch against upstream microsoft/terminal and found them clean, and confirmed the registry restore is byte-for-byte exact (before / after-register / after-unregister snapshots capturing value kinds with environment strings unexpanded). The PR is deliberately v3-only: v1/v2 are refused with Please scrutinise most:
|
Rate Limit Exceeded
|
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. |
![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: 4f1d031197
ℹ️ 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".
|
@codex review Round 2, pushed as Untrusted handoff message pointer (greptile) — fixed, and worse than reported. Delivery is
Console-half TOCTOU (codex) — recheck added, rollback declined. The commit now re-reads Please concentrate on:
Merge-order note, handled centrally: #188 merges before #164, because #164's release gates require the proxy DLL this PR packages. Out of scope but real, flagged rather than fixed: |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
|
@coderabbitai review CodeRabbit specifically: earlier requests on this PR were dropped for rate limiting, so this is your first look. Now at This PR registers noctty as a Windows default-terminal provider: an out-of-process COM server implementing Highest-value areas, all from fixes landed this round:
Out of scope, please don't report here: |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- changed files ---'
git diff --name-status origin/main...HEAD
echo '--- handoff outline ---'
ast-grep outline src/apprt/win32_terminal_handoff.zig --items all --view expanded || true
echo '--- Exec outline ---'
ast-grep outline src/termio/Exec.zig --items all --view expanded || true
echo '--- focused symbols and message references ---'
rg -n -C 4 \
-e '\bPending(Id|Session)\b' \
-e '\b(drainQueuedTerminalHandoffs|EstablishPtyHandoff|CoImpersonateClient|CoRevertToSelf|live_objects|isBusy|LockServer)\b' \
-e '\bWM_WINHOSTTY_TERMINAL_HANDOFF\b' \
src/apprt/win32_terminal_handoff.zig src/apprt/win32.zig src/apprt/win32 2>/dev/null || true
echo '--- adopted PTY and Exec references ---'
rg -n -C 4 \
-e '\b(handoff|adopted|pseudo_console|WindowsPty|signal_pipe|closeHandoffPipeCopies)\b' \
src/pty.zig src/termio/Exec.zig src/apprt/win32.zig src/Surface.zig 2>/dev/null || trueLength of output: 56169 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- pending queue, Server, and COM object lifetime ---'
sed -n '224,680p' src/apprt/win32_terminal_handoff.zig
echo '--- impersonation and recovery implementation ---'
sed -n '680,790p' src/apprt/win32_terminal_handoff.zig
echo '--- handoff message dispatch and idle revoke path ---'
sed -n '1828,1895p' src/apprt/win32.zig
sed -n '3180,3232p' src/apprt/win32.zig
sed -n '4400,4475p' src/apprt/win32.zig
sed -n '6720,6755p' src/apprt/win32.zig
echo '--- Windows PTY handoff ownership and resize ---'
sed -n '332,615p' src/pty.zig
echo '--- Exec adopted-session construction, stop, IO loop, and teardown ---'
rg -n -C 6 \
-e '\b(adopted_client_process|AdoptedSession|handoffHandles|deinit\s*\(|pub fn create|pub fn init)\b' \
src/termio/Exec.zig
sed -n '1040,1250p' src/termio/Exec.zig
sed -n '1420,1600p' src/termio/Exec.zig
echo '--- focused tests ---'
sed -n '1460,1600p' src/apprt/win32_terminal_handoff.zig
sed -n '1590,1685p' src/termio/Exec.zigLength of output: 81387 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- every adopted-session transfer and cleanup site ---'
rg -n -C 8 \
-e '\btakeAdopted\s*\(' \
-e '\badopted_session\b' \
-e '\bPendingSession\b' \
src/Surface.zig src/apprt/win32.zig src/termio/Exec.zig src/apprt/win32_terminal_handoff.zig
echo '--- App setup, App teardown, and server drain/revoke calls ---'
rg -n -C 10 \
-e '\b(terminal_handoff_server|drainPending|\.revoke\(\)|embedding_mode|CoInitializeEx)\b' \
src/apprt/win32.zig
echo '--- source at transfer candidates ---'
sed -n '1,260p' src/Surface.zig
sed -n '4475,4610p' src/apprt/win32.zig
sed -n '1680,1805p' src/apprt/win32.zig
echo '--- complete diff for reviewed Zig implementation ---'
git diff --find-renames --unified=40 origin/main...HEAD -- \
src/apprt/win32_terminal_handoff.zig \
src/apprt/win32.zig \
src/Surface.zig \
src/pty.zig \
src/termio/Exec.zigLength of output: 105361 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- current commit ---'
git rev-parse HEAD
echo '--- exact ownership-transfer references ---'
rg -n -C 12 '\btakeAdopted\s*\(' src
rg -n -C 12 '\badopted_session\b' src/Surface.zig src/apprt/win32.zig src/termio/Exec.zig
echo '--- App initialization and teardown that own terminal_handoff_server ---'
rg -n -C 16 '\b(drainPending|terminal_handoff_server|terminal_handoff_server\.|deinit\(self: \*App|pub fn deinit)\b' src/apprt/win32.zig
echo '--- Surface initialization around adopted session ---'
ast-grep outline src/Surface.zig --items all --view expanded || true
rg -n -C 20 '\b(init|create|adopted_session|Exec\.init)\b' src/Surface.zigLength of output: 96192
I found no actionable issues in the requested areas. The pending-session table has single-consumption ownership. The adopted session moves safely through The impersonation path calls The I did not run Windows execution tests in this environment.
|
|
@coderabbitai review @codex review Final pre-merge review pass. This PR has already been through adversarial review and at least one bot round; findings were triaged as fix / decline-with-evidence / follow-up rather than applied blindly, so some flagged items are deliberate design with reasoning recorded in-thread. Please focus on anything that would be wrong at merge time: stale claims in the description versus the current head, cross-PR interactions, and correctness of security-relevant paths. If you flag something previously declined, please engage with the recorded reasoning rather than restating the original finding. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- PR metadata ---'
gh pr view 188 --repo amanthanvi/noctty --json number,title,headRefOid,baseRefOid,body,url,state
echo '--- current commit and diff summary ---'
git rev-parse HEAD
git diff --name-status origin/main...HEAD
git diff --stat origin/main...HEAD
echo '--- relevant source outlines ---'
for file in \
src/apprt/win32_terminal_handoff.zig \
src/pty.zig \
src/termio/Exec.zig \
src/apprt/win32.zig \
src/Surface.zig \
src/config/Config.zig \
src/build/TerminalHandoffProxy.zig \
scripts/package-windows.ps1 \
scripts/verify-published-release.ps1 \
.github/workflows/release.yml
do
if [ -f "$file" ]; then
echo "--- outline: $file ---"
ast-grep outline "$file" --items all --view expanded || true
fi
done
echo '--- merge/base relationship ---'
git merge-base origin/main HEAD
git rev-list --left-right --count origin/main...HEADLength of output: 18832 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- handoff implementation: declarations and sensitive symbols ---'
rg -n -C 10 \
-e '^(pub )?(const|fn) .*?(Pending(Id|Session)|Server|TerminalHandoff|ClassFactory|EstablishPtyHandoff|registerDefaultTerminal|unregisterDefaultTerminal|classifyConsoleHalf|currentApartmentIsSta)' \
-e '\b(pending|next_pending|live_objects|lock_count|isBusy|take|insert|remove|CoImpersonateClient|CoRevertToSelf|RevertToSelf|OpenThreadToken|CoRegisterClassObject|CoRevokeClassObject)\b' \
src/apprt/win32_terminal_handoff.zig
echo '--- handoff implementation: complete numbered source ---'
nl -ba src/apprt/win32_terminal_handoff.zig | sed -n '1,1720p'
echo '--- application integration and message dispatch ---'
rg -n -C 12 \
-e '\b(terminal_handoff|WM_WINHOSTTY_TERMINAL_HANDOFF|drainPending|embedding_mode|maybeExit|revoke|register)\b' \
src/apprt/win32.zig src/apprt/win32/consts.zig src/apprt/win32/sys.zig src/Surface.zig src/config/Config.zig
echo '--- adopted PTY and Exec ownership / cleanup ---'
rg -n -C 12 \
-e '\b(handoff|Adopted|adopted|pseudo_console|signal_pipe|closeHandoffPipeCopies|setSize|deinit|init|destroy|terminate|kill)\b' \
src/pty.zig src/termio/Exec.zig src/Surface.zig src/apprt/win32.zig
echo '--- registration, installer, package build, and capability-contract diffs ---'
git diff --unified=30 origin/main...HEAD -- \
dist/windows/noctty.iss \
scripts/build-package-windows.ps1 \
scripts/package-windows.ps1 \
scripts/test-windows-build-capabilities.ps1 \
build.zig \
src/build/TerminalHandoffProxy.zig \
src/cli/ghostty.zig \
src/cli/ghostty_action.zig \
src/cli/register_default_terminal.zig \
src/cli/unregister_default_terminal.zig
echo '--- PR `#164` state and release-gate changes ---'
gh pr view 164 --repo amanthanvi/noctty --json number,state,mergedAt,headRefOid,baseRefOid,title,url,body
gh pr diff 164 --repo amanthanvi/noctty --name-only
gh pr diff 164 --repo amanthanvi/noctty -- \
.github/workflows/release.yml \
scripts/verify-published-release.ps1 \
scripts/package-windows.ps1 2>&1 || trueLength of output: 33910 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- handoff: COM object, authorization, queue transfer ---'
sed -n '400,680p' src/apprt/win32_terminal_handoff.zig
sed -n '735,910p' src/apprt/win32_terminal_handoff.zig
echo '--- handoff: registry reads/writes and commit/restore ordering ---'
sed -n '995,1225p' src/apprt/win32_terminal_handoff.zig
sed -n '1570,1695p' src/apprt/win32_terminal_handoff.zig
echo '--- app: handoff queue callback, message handling, shutdown/revoke ---'
rg -n -C 20 \
-e 'fn queueTerminalHandoff|queuePending|queue_session|takePending|drainPending|WM_WINHOSTTY_TERMINAL_HANDOFF|maybeExit|isBusy|\.revoke\(\)' \
src/apprt/win32.zig
sed -n '4400,4640p' src/apprt/win32.zig
echo '--- PTY: adopted handle ownership, resize, and cleanup ---'
sed -n '300,650p' src/pty.zig
echo '--- Exec: adopted-session initialization, I/O termination, and cleanup ---'
rg -n -C 16 \
-e 'AdoptedSession|adopted_session|handoff|pub fn init|pub fn deinit|fn deinit|fn stop|fn kill|CloseHandle|close' \
src/termio/Exec.zig
sed -n '1020,1265p' src/termio/Exec.zig
sed -n '1400,1635p' src/termio/Exec.zig
echo '--- surface transfer ---'
rg -n -C 16 -e 'adopted_session|takeAdopted|Exec\.init|Exec\.create' src/Surface.zig src/apprt/win32.zig
echo '--- installer and package integration ---'
sed -n '1,240p' dist/windows/noctty.iss
git diff --unified=25 origin/main...HEAD -- \
build.zig \
scripts/build-package-windows.ps1 \
scripts/package-windows.ps1 \
scripts/test-windows-build-capabilities.ps1 \
src/build/TerminalHandoffProxy.zig \
src/cli/ghostty.zig \
src/cli/ghostty_action.zig \
src/cli/register_default_terminal.zig \
src/cli/unregister_default_terminal.zig
echo '--- PR `#164` state and changed paths ---'
gh pr view 164 --repo amanthanvi/noctty --json number,state,mergedAt,headRefOid,baseRefOid,title,url
gh pr diff 164 --repo amanthanvi/noctty --name-onlyLength of output: 167857 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- pending-ID generation and all uses ---'
rg -n -C 8 \
-e '\b(next_id|PendingId|queuePending|takePending|PostThreadMessageW|WM_WINHOSTTY_TERMINAL_HANDOFF)\b' \
src/apprt/win32_terminal_handoff.zig src/apprt/win32.zig
echo '--- entropy / random identifier sources in handoff implementation ---'
rg -n -i -C 3 \
-e '\b(random|rand|bcrypt|rtlgenrandom|uuid|guid|nonce|cookie)\b' \
src/apprt/win32_terminal_handoff.zig
echo '--- PR `#164` release gate patch lines ---'
gh pr diff 164 --repo amanthanvi/noctty | rg -n -C 10 \
-e 'noctty-terminal-handoff-proxy\.dll' \
-e 'verify-published-release' \
-e 'release-scan-defender' \
-e 'scanPaths' \
-e 'Authenticode' \
-e 'Get-AuthenticodeSignature' || true
echo '--- compare PR `#164` base to head for the affected release files ---'
gh api repos/amanthanvi/noctty/pulls/164/files --paginate \
--jq '.[] | select(.filename == ".github/workflows/release.yml" or .filename == "scripts/verify-published-release.ps1" or .filename == "scripts/release-scan-defender.ps1") | {filename,status,patch}'Length of output: 57988
I found no merge-blocking issue. The pending-session table has single-consumption ownership. The sequential The The adopted-session transfer clears each prior owner. The shutdown path closes the signal pipe and local handles. It does not terminate the adopted client process. The description’s I did not run Windows runtime or release workflow validation in this environment. You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1a7de224f
ℹ️ 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".
|
@coderabbitai full review |
|
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1a7de224f
ℹ️ 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".
#130 #188 Keep the embedding server alive while clients hold factory references, roll back COM registration writes when the selection commit fails, and propagate an adopted PTY's initial resize failure through surface initialization. Mark the known nonfunctional live handoff as experimental and describe proxy registration as configuration rather than an authorization boundary.
|
@coderabbitai review |
|
@codex review |
Rate Limit Exceeded
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcf9456c1c
ℹ️ 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".
|
@coderabbitai review |
|
@codex review |
|
@coderabbitai review Please review current head |
Rate Limit Exceeded
|
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
Register noctty as a Windows default-terminal provider: a classic
out-of-process COM server implementing ITerminalHandoff3, a MIDL proxy/
stub so the interface can be marshaled into an unpackaged server, an
adopted-PTY path that wires the handed-off ConPTY handles into termio,
and HKCU/HKLM registration with exact restore.
Windows' default-terminal pipeline is two-stage: conhost activates the
DelegationConsole CLSID (IConsoleHandoff) and that OpenConsole process
activates DelegationTerminal (ITerminalHandoff). noctty implements the
terminal half only and refuses to register unless a compatible console
half is already selected, because it cannot supply one.
Only ITerminalHandoff3 {6F23DA90-...} is implemented. It is the sole
terminal-handoff IID that Windows Terminal 1.24 package-registers, each
OpenConsole build calls exactly one compiled revision with no
negotiation, and v3 is the revision that lets the terminal create its
own pipes -- which matters because v1/v2 hand over host-created pipes
whose semantics do not match the overlapped named pipe that noctty's
libxev IOCP reader requires. v1/v2 are refused with E_NOINTERFACE and
one log line naming the minimum version.
The handoff supplies no HPCON, so an adopted PTY resizes by writing the
private ConPTY signal packet (opcode 8, u16 cols, u16 rows) to the
supplied signal pipe rather than calling ResizePseudoConsole.
Cross-process marshaling needs a registered proxy/stub. Windows
Terminal's OpenConsoleProxy.dll cannot be borrowed: it lives inside the
MSIX package and loading it from an unpackaged process fails with
E_ACCESSDENIED. noctty therefore ships its own proxy generated by MIDL
from Microsoft's IDL, checked in for x64 and arm64 so the build does not
require the Windows SDK.
Caller authorization uses CoImpersonateClient rather than the marshaled
server process handle, whose access mask we do not control.
Refs #130
Four review findings against the ITerminalHandoff3 server. The wake-up message no longer carries a pointer. WM_WINHOSTTY_TERMINAL_ HANDOFF was posted to the embedding UI thread with the PendingSession address in its LPARAM, and the loop accepted any nonzero value: thread ids are enumerable and PostThreadMessageW crosses process boundaries at the same integrity level, so any process on the desktop could hand the default-terminal process an address of its choosing to dereference, deinit and free. Sessions now live in a Server-owned table and the message carries only an opaque identifier; an identifier we never issued or already consumed is dropped without touching any session. A failed CoRevertToSelf is no longer a failed handoff. The thread that serves EstablishPtyHandoff is the process message loop, so leaving it impersonating the COM caller means every later window, config read, session write and child process runs under someone else's token. Confirm the token is really still attached, try RevertToSelf directly, confirm again, and panic if it is still there. Losing the adopted windows is the cheaper failure: Windows falls back to a console window for the pending launch. Outstanding objects now keep the server alive. LockServer is optional in the COM contract, so a client that created an ITerminalHandoff3 and had not yet called EstablishPtyHandoff was invisible to the idle check and could have the class revoked and the process exited underneath it. The server counts live objects from creation to final release and reports itself busy while any exist, alongside the lock count and the queue. The console half is re-read immediately before the selection commit. DelegationConsole and DelegationTerminal are independent values and the user can change their default terminal in Settings mid-registration, so the early check can be stale by the time the terminal half is written. The recheck costs one registry read and fails before any selection write, which leaves the previous terminal selected; a transactional rollback would cost far more than the window is worth.
#130 #188 Keep the embedding server alive while clients hold factory references, roll back COM registration writes when the selection commit fails, and propagate an adopted PTY's initial resize failure through surface initialization. Mark the known nonfunctional live handoff as experimental and describe proxy registration as configuration rather than an authorization boundary.
#130 Snapshot default-terminal registry state before unregistration starts. If a later registry operation fails, rebuild owned class values, restore completed shared mappings in reverse order, and restore the terminal selection last while preserving concurrent newer owners.
#130 Keep the COM registration cookie and tracked registration references when CoRevokeClassObject fails. The embedding idle-exit path now stays alive for a later retry instead of terminating while activation may still target the class object.
#130 Replace sequential thread-message identifiers with cryptographically random nonzero capabilities. Check each candidate against live queue entries while holding the queue mutex so forged same-integrity messages cannot predict and consume an active handoff.
2a76ce1 to
01bbf48
Compare
|
@coderabbitai review Please review current head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
target='01bbf482d75370a9731b821ac72e112af883a4d1'
prior='b5784f54bfde0f1f77d4b3f58b55bfd1622427a7'
base='26f26aedd924f017b8fbe3b7eb659265bb0c3de5'
echo '--- verify commits ---'
git rev-parse "$target" "$prior" "$base"
echo '--- commits after previous review ---'
git log --oneline "$prior..$target"
echo '--- changes since previous review ---'
git diff --name-status "$prior...$target"
git diff --stat "$prior...$target"
echo '--- full focused delta since previous review ---'
git diff --find-renames --unified=100 "$prior...$target" -- \
src/apprt/win32_terminal_handoff.zig \
src/apprt/win32.zig \
src/pty.zig \
src/termio/Exec.zig \
src/Surface.zig \
test \
docs
echo '--- handoff module map ---'
ast-grep outline src/apprt/win32_terminal_handoff.zig --items all --view expanded || true
echo '--- pending capability and message flow ---'
rg -n -C 12 \
-e '\b(PendingId|PendingSession|pending_sessions|pending.*capab|capability|random|drainPending|drainQueuedTerminalHandoffs|EstablishPtyHandoff)\b' \
-e '\bWM_WINHOSTTY_TERMINAL_HANDOFF\b' \
-e '\b(PostThreadMessageW|PeekMessageW|GetMessageW)\b' \
src/apprt/win32_terminal_handoff.zig src/apprt/win32.zig
echo '--- ownership and cleanup transfers ---'
rg -n -C 10 \
-e '\b(takeAdopted|adopted_session|closeHandoffPipeCopies|PendingSession|deinit\s*\(|release\s*\()\b' \
src/apprt/win32_terminal_handoff.zig src/apprt/win32.zig src/Surface.zig src/pty.zig src/termio/Exec.zig
echo '--- test coverage for pending ids and message validation ---'
rg -n -C 8 \
-e 'test ".*(pending|handoff|capab|random|replay|message)' \
-e '\b(PendingId|PendingSession|pending.*capab|capability)\b' \
src test 2>/dev/null || trueLength of output: 173285 🏁 Script executed: #!/bin/bash
set -euo pipefail
target='01bbf482d75370a9731b821ac72e112af883a4d1'
prior='b5784f54bfde0f1f77d4b3f58b55bfd1622427a7'
echo '--- commit series and changed files ---'
git log --oneline "$prior..$target"
git diff --name-status "$prior...$target"
echo '--- exact changes after the prior review ---'
git diff --find-renames --unified=70 "$prior...$target" -- \
src/apprt/win32_terminal_handoff.zig \
src/apprt/win32.zig \
test/windows \
docs
echo '--- PendingQueue and Server complete implementation ---'
sed -n '250,450p' src/apprt/win32_terminal_handoff.zig
echo '--- handoff creation and ownership after PendingSession allocation ---'
sed -n '590,690p' src/apprt/win32_terminal_handoff.zig
echo '--- queue callback and its failure path ---'
sed -n '8020,8085p' src/apprt/win32.zig
echo '--- UI-thread terminal-handoff message dispatch ---'
rg -n -C 18 '\bWM_WINHOSTTY_TERMINAL_HANDOFF\b' src/apprt/win32.zig
echo '--- App shutdown, class revocation, and pending-session drain ---'
rg -n -C 20 '\b(drainPending|terminal_handoff_server|finishRevoke|revoke\(\)|isBusy\(\))\b' \
src/apprt/win32.zig src/apprt/win32_terminal_handoff.zig
echo '--- capability-specific tests ---'
sed -n '1850,1920p' src/apprt/win32_terminal_handoff.zigLength of output: 144580 🏁 Script executed: #!/bin/bash
set -euo pipefail
target='01bbf482d75370a9731b821ac72e112af883a4d1'
prior='b5784f54bfde0f1f77d4b3f58b55bfd1622427a7'
echo '--- commit ancestry after the prior reviewed head ---'
git log --oneline --decorate --ancestry-path "$prior..$target"
echo '--- commits that changed pending-handoff code ---'
git log --format='%H %s' "$prior..$target" -- src/apprt/win32_terminal_handoff.zig src/apprt/win32.zig
echo '--- patches for only those handoff commits ---'
for commit in $(git log --format='%H' "$prior..$target" -- src/apprt/win32_terminal_handoff.zig src/apprt/win32.zig); do
echo "### $commit"
git show --format='format:%h %s' --unified=45 "$commit" -- \
src/apprt/win32_terminal_handoff.zig src/apprt/win32.zig
done
echo '--- complete UI handler ---'
sed -n '2800,2885p' src/apprt/win32.zig
echo '--- all queue callback call sites ---'
rg -n -C 10 '\bqueueTerminalHandoff\s*\(' src
echo '--- all direct pending queue access sites ---'
rg -n -C 8 '\b(queuePending|takePending|pending\.(insert|take|drain)|nextCapabilityWithSource)\b' \
src/apprt/win32_terminal_handoff.zig src/apprt/win32.zigLength of output: 112930
I found no actionable issue in the random pending-handoff capability change.
The UI message handler treats The I did not run Windows runtime or COM integration tests in this environment.
|
|
Codex Review: Didn't find any major issues. Hooray! 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". |
… mangling catalog (#129) (#164) Fixes #129. Squash of issues/129-bundled-conpty at ecdc2c7; 4/4 review threads resolved; suite 4175/72/0; site tests pass; release gates now derive the signed payload list from one source (scripts/common.ps1) and verify the bundled ConPTY pair after publication. Closes #188's proxy-DLL release-gate residual.
Summary
Makes noctty registrable as a Windows default terminal provider: a classic out-of-process COM server implementing
ITerminalHandoff3, a MIDL proxy/stub so that interface can be marshaled into an unpackaged server, an adopted-PTY path that wires handed-off ConPTY handles intotermio, and+register-default-terminal/+unregister-default-terminalwith exact restore.Landing as partial: registration, COM activation, cross-process marshaling and clean restore are all live-verified, but a handed-off session does not yet become a visible window. Details and the exact remaining defect are under Residuals.
Refs #130Changes
src/apprt/win32_terminal_handoff.zig(new) — class factory +ITerminalHandoff3object,-Embeddingdetection, pending-session queue, registry writers with snapshot/restore, opt-in failure trace. Reuses the COM idioms inwin32_uia/com.zig,win32_toast_winrt.zig,win32_tab_drag_ole.zigand the registry style ofwin32_aumid.zig; no new COM framework.src/apprt/win32_terminal_handoff_proxy/(new) — Microsoft'sITerminalHandoff.idlplus MIDL-generated proxy/stub C checked in for x64 and arm64, so the build does not require the Windows SDK. Built asnoctty-terminal-handoff-proxy.dllviasrc/build/TerminalHandoffProxy.zig.src/cli/{register,unregister}_default_terminal.zig(new) +Actionenum entries.src/pty.zig— additive:WindowsPtycontrol state becomespseudo_console | handoff; adopted sessions resize by writing the private ConPTY signal packet to the supplied signal pipe.src/termio/Exec.zig— additive adopted-session path: no spawn, no job object, noxev.Process, never terminates the adopted client; output EOF ends the session.src/apprt/win32.zig,src/Surface.zig,src/config/Config.zig— small additive hooks for embedding mode and passing an adopted session through toExec. No new user-facing config option;-Embeddingis consumed as an argv switch.noctty.exe; Inno[Files]/[Registry]entries withuninsdeletekeyon noctty-owned keys only..github/workflows/release.ymldeliberately untouched (see Residuals).docs/windows.md"Default terminal" section,docs/status.mdand capability-matrix rows.Design notes worth reviewing
DelegationConsole(IConsoleHandoff), and that OpenConsole process activatesDelegationTerminal(ITerminalHandoff). noctty implements the terminal half only and refuses to register unless a compatible console half is already selected, since it cannot supply one.ITerminalHandoff3{6F23DA90-...}is implemented. It is the only terminal-handoff IID Windows Terminal 1.24 package-registers; each OpenConsole build calls exactly one compiled revision with no negotiation; and v3 is the revision that lets the terminal create its own pipes — which matters because v1/v2 hand over host-created pipes whose semantics do not match the overlapped named pipe noctty's libxev IOCP reader needs. v1/v2 are refused withE_NOINTERFACEplus one log line naming the minimum version.HPCON, so resize writes the private ConPTY signal packet (opcode8,u16cols,u16rows) to the signal pipe instead of callingResizePseudoConsole.ConptyPackPseudoConsoleis deliberately avoided.CoImpersonateClient, not the marshaled server-process handle whose access mask we do not control.Validation
Gates, all on this branch:
zig build -Demit-exe=true— exit 0zig build test -Dtest-filter=handoff— exit 0zig fmt --checkover all 12 changed/new Zig files — exit 0pwsh -NoProfile -File scripts/check-source-format.ps1— exit 0, "PowerShell syntax and JSON validity checks passed."pwsh -NoProfile -File test/windows/flagship/Test-VerificationContracts.ps1— exit 0, "Windows x64 baseline checker probes: PASS", "flagship verification contracts: PASS (2 scenarios)"Live, on Windows 11 26200 with
Microsoft.WindowsTerminal 1.24.11911.0installed and selected as default terminal.Retained artifacts —
evidence/130/in the shared evidence store, regenerated by the reproducibleevidence/130/Capture-HandoffEvidence.ps1(it records binary hashes, snapshots every touched registry value as raw bytes plus value kind with environment strings unexpanded, exercises COM, then restores and verifies):registry-1-before.txt/registry-2-after-register.txt/registry-3-after-unregister.txt—+register-default-terminalpreservesDelegationConsoleand flipsDelegationTerminalfrom WT's{E12CFF52-...}to noctty's{33368C6F-...}, creating both CLSID keys and all threeInterface\{IID}\ProxyStubClsid32mappings.+unregister-default-terminalrestores the prior value and removes them. File 3 is byte-for-byte identical to file 1 (the script asserts this and reportsRESTORED EXACTLY).transcript.txt— COM activation returns an object and starts onenoctty.exe; cross-processQueryInterfacereturnshr=0x00000000forITerminalHandoff3 {6F23DA90-...}andhr=0x80004002(E_NOINTERFACE) for both{59D55CCE-...}and{AA6B364F-...}, which is exactly the v3-only contract this PR describes.Exercised in this session but no artifact retained (recorded here so a reader knows the difference):
QueryInterfacereturnsE_NOINTERFACEwithout our proxy registered, and WT'sOpenConsoleProxy.dllfails to load into an unpackaged process withE_ACCESSDENIED— the reason we ship our own proxy. TheS_OKhalf of this is captured above; the negative controls are not.cmd.exedrove the full chain:OpenConsole.exe -Embeddingstarted, noctty started ~0.3 s later, and OpenConsole loadednoctty-terminal-handoff-proxy.dllwithin ~1 s.cmd.exehanded off into the running WT instance, confirming the trigger and the pipeline are valid.The machine is restored to its exact original registry state after every capture run, and the restore is verified rather than assumed; no
%%Startup, CLSID or Interface values are left behind.Residuals / user steps
EstablishPtyHandoffno longer takes any refusal path (the failure trace stays empty), but two noctty processes briefly appear, no window shows, andcmd.exeexits on its own — i.e. the session is adopted and then torn down. Two things to chase: why a second process appears at all (the-Embeddingbranch may be falling through into normal startup / single-instance forwarding), and premature closing of the handed-off pipe or signal handles (likely thecloseHandoffPipeCopiestiming relative to COM marshaling, or the adoptedPtybeing copied rather than moved into the surface). SetNOCTTY_HANDOFF_TRACE=1to get%LOCALAPPDATA%\noctty\handoff.log.com.microsoft.windows.console.host/com.microsoft.windows.terminal.hostapp-extension catalogs and accepts a pair only when both come from the same package, so an unpackaged noctty can be selected by registry but cannot appear in the Windows Settings or Windows Terminal picker. Selecting anything in the picker overwrites our pair. Package identity (sparse or full MSIX) would be required; not attempted here.IConsoleHandoff.Microsoft.VisualStudio.Component.VC.Tools.ARM64.noctty-terminal-handoff-proxy.dllis in both the portable zip and the installer, and it is loaded in-process by OpenConsole, but it is excluded from Authenticode re-verification (release.yml:337,verify-published-release.ps1:179) and from the Defender scan list (release.yml:388-392).package-windows.ps1does sign it and the capability manifest covers it, so this branch's manifest/count invariants are intact (the$scanPaths.Count -ne 8assertion still yields 8). Wiring it into both release gates must land with feat(pty): own the ConPTY layer with a bundled OpenConsole + measured mangling catalog (#129) #164 before a release ships this DLL; it is not a nice-to-have deferral.ISCC.exeunavailable); the static and flagship contracts pass.Stacked on
Nothing — branched from
main.src/pty.zigis additive and does not depend on #164's bundled-ConPTY resolver, but both touch that file, so expect a small merge.Review R-188 dispositions
Fable review verdict was approve-with-changes; the GUID audit came back clean (all three
ITerminalHandoffIIDs verified against upstream at the pinned commit, MIDL output authentic, no injected code). All blocking and medium items are addressed in this branch:classifyConsoleHalfpanicGUID.parseNoBracesasserts dashes at 8/13/18/23 rather than erroring, so a 38-char braced dash-less value panicked (UB in ReleaseFast) instead of reaching the.invalidarm. Dash positions are now validated before the call. New negative test covers same-length/wrong-dash, misplaced-dash, trailing-dash and bad-hex shapes.[UninstallRun]runs+unregister-default-terminal(runhidden,RunOnceId) before file removal, so the uninstalling user'sDelegationTerminaland proxy mappings no longer point at deleted files.docs/windows.mdnow say the check refuses a missing/empty/inbox console half, and the docs state plainly that a stale CLSID from an uninstalled Windows Terminal is accepted and will degrade console launches.LockServerwrite-only vs 5s idle timerServer.isLocked()is now honored: the idle path refuses to exit while a client holds the class object, and it revokes the class object before deciding, then re-checks for a lock or a queued handoff and re-registers if anything landed in the gap. The last-window-close path goes through the same helper instead of callingPostQuitMessagedirectly.docs/windows.mddescribes the phishing shape, records that the per-userProxyStubClsid32gating means an unregistered machine is not exposed, and states that IL-mismatch rejection is correct by design so it is not later "fixed".currentApartmentIsSta()and refuses to close the marshaled copies outside an STA rather than silently corrupting the handoff; the constraint is also written into the docs section above.setSizeafter shutdownTrim list: applied 1-3, 5-7 and 9-10 (
lock_countnow honored rather than deleted; deadclass_registeredfield removed; duplicatesavePreviousInterfaceProxycall dropped; the operation-enum arrays and their near-tautological ordering test inlined into straight-line code with the intent kept as comments; the@typeInfosignature test dropped; thesavePreviousTerminalwrapper inlined; the proxy README'sbuild.zigattribution corrected tosrc/build/TerminalHandoffProxy.zig;Config.zignow useserror.SkipZigTestinstead of silently passing on Linux CI). Item 4 applied: the[Files]Excludes:plus secondSource:line collapsed to one recursive line. Item 8 resolved by deleting the unreferencednoctty-terminal-handoff-proxy.defso the two export lists cannot drift, withexports.cdocumented as the single source. The proxy README also now notes the uniform2038MIDL timestamp flagged in the audit.Rebase
Rebased onto
origin/main5220df49e. The only conflicts were with #177's split ofwin32.zigintowin32/consts.zig,win32/sys.zigand friends. Resolution kept main's structure: the constants block moved toconsts.zig(WM_WINHOSTTY_TERMINAL_HANDOFF), the newCoGetApartmentTypeextern andAPTTYPE*values moved tosys.zig, and this branch's call sites were rewritten to thesys./c.prefixes. Verified against a preserved pre-rebase branch: every other touched file is byte-identical, and the only lines lost from thewin32.zigdelta are exactly the nine declarations that were re-homed intoconsts.zig/sys.zig.