open ssh into the VM for a remote MCP client by glslang · Pull Request #34 · glslang/agent-sandbox-vm

glslang · 2026-08-21T06:39:56Z

Summary

Scripts the "remote phase 0" arrangement: an MCP server that has to run on Windows — a debugger, above all — does not have to run where the harness driving it runs. ssh already carries a pair of pipes, so a stdio MCP server registered on the client as ssh -T HOST SERVER.EXE needs no transport code on either side, and a client disconnect still releases whatever the server was holding, because closing the channel closes the remote stdin.

New VM-side script scripts/Setup-RemoteMcp.ps1, copied into the VM by Start-Provision.ps1 alongside the kernel debugging scripts. Nothing runs it during provisioning — it opens an inbound port, so it stays opt-in.

powershell -ExecutionPolicy RemoteSigned -File C:\Setup-RemoteMcp.ps1 `
    -PublicKey "ssh-ed25519 AAAA... you@client" `
    -ClientAddress 10.211.55.2 `
    -ServerCommand C:\tools\windbg-mcp.exe

The run ends with the ~/.ssh/config block, the handshake probe, and the claude mcp add line to paste on the client, filled in with this VM's address and the resolved user.

What it configures, and why each one is scripted rather than described

Four things have to be true, and three of them fail silently:

The firewall. The rule the OpenSSH capability installs covers Domain and Private only, and a hypervisor's guest network usually comes up Public — so sshd is running and listening and looks correct from inside the guest while the client's SYNs are dropped. That presents as ssh hanging right after the "Connecting to ... port 22" line, where nothing listening at all would have said Connection refused at once. The script adds its own rule scoped to -ClientAddress, and — because allow rules are additive — deals with every other inbound rule that could reach sshd. It narrows but never widens: a rule scoped to Any becomes the allowlist, one already inside it is left alone and not recorded, one that overlaps is cut to the overlap. Anything it cannot resolve — a rule it may not edit, a LocalPort Any rule that reaches sshd from outside the requested scope, a scope it cannot compare without guessing at containment — fails the run rather than reporting a boundary it did not enforce. -SkipFirewall is the deliberate way past.
The default shell. sshd runs an exec request through it, and only cmd.exe leaves line-delimited JSON-RPC alone: PowerShell applies its output encoding to a native command's stream and serializes its progress and error streams as CLIXML on stderr. DefaultShellCommandOption is corrected alongside it when set — a machine configured for a Unix-style or PowerShell shell carries -c or -Command there, and sshd would then run cmd.exe -c ....
Which authorized-keys file. sshd ignores ~/.ssh/authorized_keys for a member of Administrators — which the VM user is if you are kernel debugging — and reads C:\ProgramData\ssh\administrators_authorized_keys. A key in the other file is not an error anywhere; it is just a login that never authenticates. Membership is resolved through nested groups; where a group cannot be expanded at all (a domain group), the run refuses to guess and -AuthorizedKeysFile Administrators|PerUser settles it.
Its ACL. sshd refuses an authorized-keys file any other principal can write and logs the refusal nowhere obvious. The DACL is rebuilt rather than amended — icacls /inheritance:r /grant drops only inherited entries and touches only the trustees it names — and grants go in by well-known SID, so it works on a non-English Windows.

Reversal: one ledger, four rules

Every change goes into a ledger in C:\ProgramData\agent-sandbox\remote-mcp-ssh.json — a competing firewall rule's scope, profile and enabled state; the previous DefaultShell and its command option; the sshd startup type and running state; the DACL of each authorized-keys file rebuilt; each key installed; and the newline added to a file that had none. One entry per change, carrying what the machine had (Original) and what this script wrote (Applied), under one set of rules for all of them:

Record before mutating. A change made and not written down is one -Disable can never put back. A run that fails half way still leaves -Disable able to undo what it did; a run that changes the machine and then cannot write the record fails rather than reporting success.
Track what was applied, not what was planned. Applied starts equal to Original and advances only as each individual write returns — so it describes the machine as the script actually left it, however far through a multi-field change it got, on the way in and on the way back out alike. A rerun contributes only the fields it actually wrote; one it merely looked at keeps what the earlier run recorded, so a value somebody else set in between is never taken for this script's own.
Verify before reverting. A field goes back only where the machine still holds the value this script wrote. A shell, a firewall rule, the sshd service or a DACL that somebody changed after setup is left to them, and the run names the field that moved — putting a broad original back over a deliberate tightening would hand out access at the moment the script is taking its own away.
Drop what is restored, keep what is not. What stays on record is exactly the work still outstanding, so a later -Disable resumes rather than re-applies.

Restore order is declared by entry kind, and within one file the lower kind goes first — which is what puts a key out before that file's terminator, and both before the DACL protecting them, since removing a key needs the write access the DACL restore gives away.

This structure replaced five separate implementations of those four rules — one per component, with five record shapes, four hand-written comparisons, and none at all for the DACL. That is what fifteen rounds of review kept finding: not one broken rule, but the same rule missing from whichever component nobody had yet written it into. Adding a component meant reimplementing all four, and forgetting one was silent. A component now supplies two things — how to read its current values, and how to write one of them.

Other properties that survive from before: a record that exists but cannot be parsed stops the run before it changes anything rather than being taken for no record at all; -Disable exits non-zero when a restore actually failed, so automation can tell that from a clean close, while a -Skip switch is a choice and does not; both files this script owns are written by swap, never in place. The authorized-keys file is byte-preserving in both directions — other keys, comments, blank lines and non-ASCII comment fields survive, down to the file's exact ending, however many runs appended to it. Keys are matched the way sshd reads them, on type and base64 blob, with the type found rather than assumed to be the first field, so neither a comment edited by hand nor an entry hardened with restrict or from="..." hides a key from -Disable. A file starting with a byte-order mark is refused rather than appended to. -Disable leaves the OpenSSH capability and its host keys installed, since removing them changes the fingerprint every client has accepted.

Affected host platform

Windows / Hyper-V path only. The Parallels workflow under macos/parallels/ provisions no Setup-* scripts and is untouched.

Validation

No Windows VM was available to this session, so this is not validated end to end on a real guest. The Windows-only half — Add-WindowsCapability, the Net*Firewall* cmdlets, the HKLM:\SOFTWARE\OpenSSH writes, and the Get-Acl/Set-Acl DACL rebuild — is unexercised and wants a disposable VM before it is relied on.

What was checked, under PowerShell 7.4.6 on Linux, plus OpenSSH 9.6 for the config claims — 29 suites, all passing:

Both changed scripts parse clean ([Parser]::ParseFile, zero errors).
Unit suites over the portable logic: the firewall port classifier (exact, range, Any, service keywords, lists, empty and null filters); the administrator-membership walk (direct, nested, two-deep, a cycle, non-member readable, non-member with a domain group, member found despite one, Administrators unreadable); client-shell escaping, each case through a real /bin/sh with a stub ssh reporting the argument it was handed; byte preservation across add-and-remove for LF, CRLF, no-final-newline, non-UTF-8, empty and blank-line files — all six byte-exact; key parsing and options-aware identity matching; and the ledger's own merge, ordering and atomic save.
The generated ~/.ssh/config fed to real ssh -G for a plain path, one with a space, one with an apostrophe, and two containing a double quote — all accepted and resolving to the intended path. The unquoted and naively quoted forms are rejected by the same parser, which is what prompted the quoting and then the escaping; an alias containing a space is rejected by ssh whatever the quoting, which is why the script refuses one up front.
Flow suites driving the real script against stubbed Windows cmdlets: an invalid -User throws with zero mutations logged and no record written; failures at each stage still record what preceded them; a competing rule that refuses to narrow, or reaches sshd from outside the requested scope, fails the run with no key installed; and -Disable restores, declines or defers correctly across a skipped shell, a failed restore, a half-finished restore retried, -SkipKeys, a corrupt record, an undeletable record, and a shell / rule / service / key that somebody else changed after setup — including one changed between two enable runs. Exit status is 0 for a clean close, 0 for a deliberate skip, 1 for a real failure.

VM lifecycle impact

None on its own: no new provisioning step, no snapshot change. When run, it starts sshd and sets it to start automatically, so the state persists into a snapshot taken afterwards.

Review history

Seventeen rounds of automated review, fifty findings, each verified against the code before being fixed. The great majority of the later rounds landed in the rollback bookkeeping, and several were regressions from the previous round's fix to it. The sixteenth round is where patching them individually stopped being the right answer: three of its five findings were the same missing rule in three more components, so the bookkeeping was replaced by the single ledger above rather than patched a fourth time. The ACL's missing check — a gap flagged in an earlier round and left open — closed with no code written for it. The seventeenth round found one P1 in that ledger: the rerun merge replaced the applied map wholesale, which let a value an administrator had set be recorded as this script's own and then reverted. That is rule 2's second sentence, and it is fixed.

Two things still worth a reviewer's attention more than any individual fix: the ledger is the part to read, and to read for its shape; and the Windows-only half above has never run on a real guest.

An MCP server that has to run on Windows -- a debugger, above all -- does not have to run where the harness driving it runs. ssh already carries a pair of pipes, so a stdio MCP server registered on the client as `ssh -T <host> <server.exe>` needs no transport code on either side, and a client disconnect still releases what the server was holding because closing the channel closes the remote stdin. Setup-RemoteMcp.ps1 does the four things that have to be true for that, three of which fail silently and so are worth scripting rather than describing: - The OpenSSH capability's own firewall rule covers Domain and Private only, and a hypervisor's guest network usually comes up Public -- sshd is running and listening and looks correct from inside the guest while the client's SYNs are dropped, which presents as ssh hanging rather than refusing. The script adds its own rule scoped to -ClientAddress and, as Setup-KernelDebuggee.ps1 already does for WinRM, narrows every other inbound rule on that port to the same allowlist, since firewall allow rules are additive. - sshd runs an exec request through the default shell, and only cmd.exe leaves line-delimited JSON-RPC alone: PowerShell applies its output encoding to a native command's stream and serializes its progress and error streams as CLIXML on stderr. - sshd ignores ~/.ssh/authorized_keys for a member of Administrators, which the VM user is if you are kernel debugging, and reads the ProgramData file. A key in the other file is not an error anywhere, just a login that never authenticates. - sshd refuses an authorized-keys file any other principal can write and logs the refusal nowhere obvious, so the ACL is set by well-known SID rather than by group name, which also survives a non-English Windows. Prior firewall scope, default shell, sshd startup type and installed keys go to C:\ProgramData\agent-sandbox\remote-mcp-ssh.json so -Disable puts them back exactly, and a rerun of enable never overwrites that record. -Disable leaves the capability and its host keys installed: removing them would change the fingerprint every client has already accepted. Start-Provision.ps1 copies it into the VM alongside the kernel debugging scripts. Nothing runs it during provisioning -- it opens an inbound port, so it stays opt-in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

coderabbitai · 2026-08-21T06:40:04Z

Important

🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e0bde71b-d576-4670-b3f0-7893c2e7ff8b

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 67b86dbe5b

ℹ️ 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".

Three review findings on Setup-RemoteMcp.ps1, each one a way the rollback record could end up describing a machine that no longer exists. A component this script never wrote is not one -Disable may revert. On a first enable with -SkipDefaultShell the prior value was recorded as null, which -Disable reads as "delete it" -- so it removed a DefaultShell someone else had configured. The record now tracks whether the script wrote each component, and -Disable leaves an unmanaged one alone. A later run that does write it captures the value it found, since the skipped run changed nothing. Resolving the login account happens before anything is mutated. Both lookups throw for an account that does not exist or has never signed in, and they sat after sshd, the firewall and the default shell had been changed but before any of that was written down -- so -Disable could not put it back, and the next enable captured the modified values as the originals. The enable body now runs under a try/finally that persists whatever it had changed, and the firewall helper appends each narrowed rule as it goes rather than returning them at the end, so a failure creating this script's own rule cannot lose them. -Disable keeps the record unless everything really was put back. It caught restore failures, warned, and then deleted the only way to retry. A failed restore or a -Skip switch now keeps the file and the run says what is still outstanding. Found while testing the above: @($list) on a System.Collections.Generic.List throws "Argument types do not match", and a Mandatory parameter refuses an empty collection -- which is what every first run passes. The collectors are ArrayLists and the parameter carries AllowEmptyCollection. Verified by driving the script on Linux against stubbed Windows cmdlets: an invalid -User throws with zero mutations logged and no state file; a simulated New-NetFirewallRule failure after a narrowing still records that narrowing; a -Disable after -SkipDefaultShell leaves the machine's own shell in place while one after a normal enable restores it; and a failing restore, like -SkipKeys, keeps the state file while a clean run removes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d85c1751d

ℹ️ 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".

Second review round on Setup-RemoteMcp.ps1. Five findings, all real; three of them were a check reporting a stronger guarantee than it enforced. A port filter is not always an exact number. -LocalPort takes a single port, a range or a keyword, so testing string equality against "22" missed a rule whose range spans it and missed "Any" outright -- either one goes on holding the port open at its own scope while the run reports -ClientAddress as the boundary. The filter is now classified: a rule covering the port exactly or by range is narrowed like before, and one whose filter is LocalPort Any is reported rather than rescoped, because such a rule is usually scoped to a program rather than a port -- an app's inbound rule, not an ssh hole -- and narrowing every one of them to the ssh client would take the VM's other inbound traffic with it. A filter naming no protocol counts as TCP too. The authorized-keys DACL is rebuilt rather than amended. icacls /inheritance:r drops only inherited entries and /grant only adds or updates the trustees it names, so an explicit entry for anyone else survived both: sshd would go on refusing the file, or another account would keep write access to the login keys, while the run said the ACL was tightened. That was the only caller of Invoke-NativeCommand, which goes with it. Administrator membership is resolved through nested groups. sshd reads it off the logon token; Get-LocalGroupMember returns direct members only, so a user who is an administrator through a group got a confident "no", the key went to the per-user file while sshd read the administrators one, and every login failed. A group that cannot be expanded at all -- a domain group among the members -- stays undetermined and keeps the existing assumption, and the summary names the file it chose either way. Two more places recorded a change only after it could no longer be recorded. Start-SshService returned the prior service state after Start-Service, which throws on an invalid sshd_config with the startup type already changed, so the finally block persisted nothing and -Disable left sshd Automatic. It and Set-SshDefaultShell now write into the record before they touch anything. And -Disable stopped sshd with -ErrorAction SilentlyContinue, so a failure to stop printed "sshd stopped", never reached the catch, and let the state file be deleted with the service still running. Verified on Linux against stubbed Windows cmdlets: 15 cases over the port classifier, including the range and Any filters the old test missed; a bench whose competing rules are an exact 22, a 20-30 range, a LocalPort Any app rule and an unrelated 5985 narrows the first two, warns about the third and leaves the fourth alone; a thrown Start-Service still records the prior startup type; and a failing Stop-Service is now visible and keeps the record for a retry. The DACL rebuild needs Windows-only APIs and is stubbed out there -- it is unverified, like the rest of the Windows-only half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5bdb839f9f

ℹ️ 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".

Third review round on Setup-RemoteMcp.ps1. Four findings, all real. Rebuilding the authorized-keys DACL is destructive -- an explicit entry another account or a management tool relied on goes with it -- and only the key lines were recorded, so enable-then-disable quietly revoked access the machine had before. The DACL being replaced is now captured before the rebuild and restored by -Disable, after the keys are removed, since removing them needs the write access being given away. Only the DACL: the owner and the audit entries are not touched, so putting them back is not this script's business. A file this run created has no prior DACL and records none, and -SkipKeys leaves the ACL alone because the keys it protects are still there. Administrator membership answers Unknown instead of assuming. Bailing out of the walk on a group that cannot be expanded was worse than the bug it replaced: domain groups sit in Administrators on any domain-joined machine and none of them are local groups, so every account on such a machine -- standard users included -- would have been called an administrator and had its key filed where sshd was not going to look. The walk now carries on past what it cannot read and reports Unknown only if the user was not found anywhere it could. Auto then stops before changing anything and names the group; -AuthorizedKeysFile Administrators or PerUser settles it by hand. A rule can be half-narrowed. Address and profile are two calls, and the second throwing with the first applied left the rule changed with its original address written down nowhere. The record goes in before the first call now, like the service and the shell already do. The printed remote command is quoted so the quotes survive. ssh joins its remaining arguments into one string for the remote shell, so the client shell's own quotes are long gone by then and a -ServerCommand containing a space reached cmd.exe bare, which ran the part before the first space. Verified on Linux against stubbed Windows cmdlets: 8 cases over the membership walk, covering direct, nested and two-deep membership, a cycle, a non-member with everything readable, a non-member with a domain group present, a member found despite one, and Administrators itself unreadable; Auto refusing with zero mutations logged and both explicit values choosing the right file on a bench where it cannot decide; the prior DACL captured, restored on -Disable, absent for a file the run created, and left alone under -SkipKeys; and a rule whose profile change throws still recorded. The DACL rebuild and restore themselves need Windows-only APIs and stay stubbed, so they are unverified along with the rest of the Windows-only half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 00a8972b3a

ℹ️ 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".

Fourth review round on Setup-RemoteMcp.ps1. Three findings, all real, and the first of them broke the ordinary case. A VM with one NIC printed "HostName 1". Get-VMIPv4Address ends by wrapping its result, but the pipeline unrolls a one-element array back into a bare string on the way out, and the caller indexed it -- so the ssh_config block for a single-NIC guest, which is most of them, named the first character of the address. Wrapped at the call site instead. Setting DefaultShell is only half of it. A machine configured for a Unix-style or PowerShell default shell carries -c or -Command in DefaultShellCommandOption, and changing only the shell left sshd running `cmd.exe -c ...` -- cmd wants /c, so every exec request would have failed while the run reported the shell configured. The value is recorded, corrected when it is present and wrong, and restored with the shell on -Disable. Absent stays absent: /c is sshd's own default for cmd, so there is nothing to fix. The authorized-keys DACL record was a single slot. A rerun for a different -User or -AuthorizedKeysFile rebuilds a second file's DACL, and the merge kept the first record and dropped the second, so -Disable put one file back and left the other stripped of the entries it had. Records are keyed by path now and merged the way installed keys already were. Verified on Linux against stubbed Windows cmdlets: a one-address bench prints the whole address; a bench carrying -Command has it corrected to /c on enable and both halves put back on -Disable, while a bench with no option value keeps none; and two enables over two different key files record both DACLs and restore both. Every earlier suite re-run clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c7bc0d5ba

ℹ️ 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".

Fifth review round. One finding, and the fix belongs where the quoting rule lives rather than in the text beside it. A server path containing an apostrophe -- C:\Users\O'Brien\... is the ordinary way to get one -- closed the surrounding single quotes early, so both the probe and the claude mcp add line failed while the client shell was still parsing them, before ssh ran at all. The outer quoting is applied by an escaper now (' becomes '\'') rather than written into the here-string, so the two layers stay separate: single quotes get the argument past the client shell in one piece, and the double quotes inside are what survives ssh joining its arguments and reaches cmd.exe. Verified by running the printed line through a real POSIX shell with a stub ssh that reports the argument it was handed: six paths -- plain, with a space, with an apostrophe, with both, the placeholder, and one carrying $ and a backtick -- all arrive as the intended single argument. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c431dc5acb

ℹ️ 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".

Sixth review round. Three findings, each one this script overstepping what it was asked to change. An unreadable rollback record was indistinguishable from no record. A truncated or corrupt state file came back as $null, the next enable took that for a first run, and the merge overwrote it with the values this script had already written -- the machine's originals gone for good. Parsing failure throws now, and the read happens during pre-flight, so an unreadable record stops the run before it changes anything. -Disable says the same thing in its own terms: it removes only its own rule and keeps the file for a later attempt, rather than reporting there was nothing to put back. Adding a key rewrote the whole file. Every existing line went through a trim, a blank-line filter and an ASCII re-encode, so another key's non-ASCII comment was replaced with question marks and blank lines vanished -- content nothing here records, so -Disable could not put it back either. The key is appended now, through .NET so the encoding is UTF-8 with no BOM (Set-Content's UTF8 emits one, which in front of the first key stops sshd reading it, and its default is UTF-16 which sshd cannot read at all). A file whose last line has no newline gets a separator first, checked as bytes. Removal has to rewrite, so it preserves everything it is not removing. A multi-line -PublicKey was accepted as one key. The pattern is a prefix match, so a second key on a second line rode along: both were written to the file and recorded as a single multi-line entry, which -Disable then matched against no line at all and removed neither, leaving ssh access authorised after a run that said it had closed it. A line break in -PublicKey is refused. Verified on Linux against stubbed Windows cmdlets: a truncated record stops enable with zero mutations and is not overwritten, and -Disable keeps it and says why; a file carrying a comment, a non-ASCII key comment, a blank line and no trailing newline survives an add and a remove intact, with no BOM and the key not glued to the previous line; and two keys pasted together are refused with LF or CRLF while one key, padded or with a trailing blank line, is accepted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6b15c6e793

ℹ️ 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".

Seventh review round. Two findings, both consequences of earlier fixes in this branch rather than of the original script. -Disable kept the whole record whenever anything was outstanding. The retention added two rounds ago is right, but it retained components it had just restored along with the ones it had not: after -Disable -SkipKeys put the shell, the service and the firewall back, the record still claimed them, so if an administrator set a default shell while access was closed, the next -Disable would restore the value from before the first enable over it. The disable path now builds what is still changed as it goes and writes back only that, so a retry retries only what is outstanding and a component that is back is never re-applied. A record that could not be written reported success. The catch around the save was there so a failure to record could not replace the exception that brought us into the finally -- but with no such exception it swallowed the failure too, and the run printed "Remote MCP setup ready" and exited 0 with the firewall, the service, the shell and the keys all changed and nothing written down to undo them. It still steps aside for an exception already on its way up; otherwise it now fails the run. Verified on Linux against stubbed Windows cmdlets: after -Disable -SkipKeys the record holds the keys alone -- no shell, no rules, no service -- and a default shell set by someone else afterwards survives the final -Disable, which then removes the record; a failed rule restore keeps the rules and drops the shell and service; and a state path that cannot be written fails the run with the setup having visibly happened, rather than reporting it ready. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d22c0ac11a

ℹ️ 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".

Eighth review round. Three findings: one a follow-on from the last round's append fix, two present since the first commit. Removing a key still normalised the file. Appending stopped rewriting it two rounds ago, but removal went on decoding the whole file as UTF-8 and rejoining its lines with the platform newline, so an LF file came back CRLF and a comment in some other encoding came back as replacement characters -- enable then disable did not give the file back. Removal now works on bytes: it cuts out the managed line's own bytes and copies every other byte through untouched. The one thing it cannot give back is a missing final newline, because appending a line to a file that has none has to terminate it first. The record was written over itself. Set-Content opened the destination and wrote in place, so an interrupted write -- a full disk, a reset VM -- left truncated JSON where the only account of the machine's original firewall scope, shell, service and keys had been. It is written beside the record and swapped in now, so a failed write costs the update rather than the record. The printed client config named a key nobody promised. -PublicKey takes a public half from anywhere, but the ssh_config block always said IdentityFile ~/.ssh/id_ed25519; with BatchMode yes, a mismatch is a silent authentication failure rather than a prompt. -ClientIdentityFile names it, with that path as the default. Verified on Linux, where all three are testable in full: six files -- LF, CRLF, no final newline, a Latin-1 byte in a comment, empty, and blank lines only -- come back byte-for-byte after an add and a remove, bar the terminator noted above; a save whose write fails leaves the previous record intact and readable; and both the default and an explicit -ClientIdentityFile reach the printed block. Writing the swap turned up one of its own on the way: PowerShell binds $null to a .NET string parameter as an empty string, so File::Replace's backup argument needs [NullString]::Value or every update after the first throws. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4adc98a64b

ℹ️ 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".

Ninth review round. Three findings: one on the claim this script exists to make, two on last round's own additions. A competing exception that would not narrow was only a warning. Firewall allow rules are additive, so while one of them still names a wider scope sshd is reachable from outside -ClientAddress no matter how narrow this script's own rule is -- and the run went on to print "Remote MCP setup ready" with the requested remotes beside it, which is the one thing this step must never say when it is not true. A rule that cannot be narrowed now fails the run, naming it, after every narrowing that could be done has been and this script's own rule exists, so the record is complete and -Disable can put it all back. -SkipFirewall remains the way to proceed deliberately with the wider exposure. An identity path with a space produced an invalid ssh_config line. -ClientIdentityFile arrived last round unquoted, and ssh_config splits an unquoted value on whitespace, so the whole file is rejected and every printed command fails before authentication. The value is quoted now. Removing a key wrote the file in place. Rewriting it byte for byte arrived last round without the swap that the record got in the same commit, and unrelated keys and comments in that file are recorded nowhere -- a half-written file would lose them with nothing able to put them back. Both files now go through one Set-FileContentAtomically, which also keeps the destination's ACL: for an authorized-keys file that is the tightened one this script put there. Verified: openssh-client 9.6 installed here to check the config claim against the real parser rather than from memory -- the unquoted form is rejected with "keyword identityfile extra arguments at end of line", the quoted form parses and still expands ~, and the block this script emits was fed to ssh -G verbatim for a plain path, a path with a space and a path with an apostrophe, all three accepted. A rule stubbed to refuse narrowing fails the run with the key not installed and both rules still recorded. The six byte-preservation files still round-trip, now through the atomic write, with no temporary files left behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 16fed00975

ℹ️ 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".

Tenth review round. Three findings. A rule on any local port is now asked, not assumed about. Leaving those alone rested on "they are usually scoped to a program rather than a port" -- true, and still the reason not to rescope them, but a probabilistic argument standing in for a question that can simply be put. Their program and service filters are read now: one bound to something that excludes sshd is ignored by name, and one that is unbound, or bound to sshd itself, fails the run alongside the rules that would not narrow. Either way -ClientAddress is not the boundary the summary would have reported, which is the thing this step must not get wrong. An authorized-keys file with a byte-order mark is refused. Windows PowerShell's redirection writes UTF-16 by default and Get-Content decodes it happily, so the duplicate check passed while appending UTF-8 bytes onto a UTF-16 stream produced a file in two encodings that sshd cannot parse at all -- with the run reporting success over it. Any BOM stops the run now, naming the encoding, because in every case the file is already not being read the way it looks. An ACL is handed back only once the key it protects is gone. -Disable removed keys and then restored DACLs unconditionally, so a file whose key removal failed got its original DACL back with a live login key still in it: if that DACL let another principal write, the failed disable left them able to edit credentials. Those files keep the tightened ACL and carry both records forward instead. Verified on Linux against stubbed Windows cmdlets: a blanket rule bound to another program lets the run proceed and is named as ignored, while the same rule unbound, or bound to sshd.exe, fails it with no key installed and both rules still recorded; UTF-16 and UTF-8-BOM key files are refused and a plain UTF-8 one is still appended to; and a file whose key removal fails keeps its tightened ACL, with the restore never called and both records carried forward. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

The README edits belonging with e8fc54e did not apply: the passages had been reworded in earlier rounds, the patch matched nothing, and the commit went ahead without them. The same three changes, described where they belong -- what a blanket firewall rule is now asked, that a key file with a byte-order mark is refused, and that an ACL goes back only once the key it protects is gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e45d56d0ed

ℹ️ 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".

Eleventh review round. Two findings, both on last round's and round seven's own additions. A blanket rule inside the requested scope now widens nothing and is ignored. Last round taught the check to read a rule's program and service filters, but not its address and profile ones -- so a LocalPort Any rule already scoped to exactly -ClientAddress failed the run, and the error told the operator to narrow a rule that was already as narrow as it was being asked to be. Its address and profile filters are read too now. Containment is decided by exact membership rather than by working out whether one CIDR sits inside another, which can only fail to clear a harmless rule, never clear a harmful one. A record can outlive the change it describes. The disable path prunes what it restored and writes the reduced record back, but the writer leaves the previous file intact when it cannot swap the new one in -- right for the record, and it leaves a file that over-describes the machine, listing components already put back. Two answers. Restoring the default shell now checks that what is there is still what this script wrote, so a stale record cannot put a pre-enable value over a shell somebody set since. And a reduced record that cannot be written says so in those terms, rather than leaving it to be discovered by the next -Disable. The general case -- a firewall rule or an ACL changed by hand between a partial disable and a retry -- is covered by that message rather than by a per-component check. Verifying a DACL or a rule scope against what this script wrote is a larger piece of machinery than the narrow window justifies, and the shell is where a hand edit is actually likely. Verified on Linux against stubbed Windows cmdlets: a blanket rule reaching sshd from Any still fails the run; the same rule scoped to the requested address is ignored by name and the run completes; scoped to the address but sitting on a profile outside the requested set, it fails again; and a default shell changed by someone else between enable and disable survives -Disable, with the run saying it left it alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d49dd80a2

ℹ️ 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".

Twelfth review round. Three findings: two on code that has been here since the first commit, one on last round's guard. The narrowing loop widened rules. Its own comment says "only ever narrow -- widening someone else's rule to match a broader request would hand out access this script was never asked to grant", and the profile logic honoured that while the address filter was assigned unconditionally: a rule scoped to 10.0.0.5, under a run asking for LocalSubnet, came out wider than it went in. The address is now only written where the result is demonstrably narrower, which can be settled without subnet arithmetic -- "Any" is the universal set, a rule naming only addresses from the request is left alone, one that overlaps is cut to the overlap. A scope that cannot be compared without guessing is left untouched and fails the run, because the alternatives are widening someone else's rule or claiming a boundary this script cannot show it enforced. -Disable reported success whatever happened. A failed firewall restore, key removal, ACL restore, service restore or record write left the work outstanding, said so on stderr, and then printed that access was closed and exited 0 -- indistinguishable, to anything driving it, from a clean close. It exits 1 now, and says which it is. A -Skip switch is a choice rather than a failure and does not affect the status. Last round's shell guard read one value and missed two cases. An administrator who deleted DefaultShell had removed exactly what this script wrote, but a null current value did not trip the check, so -Disable reinstated the pre-enable shell over that deletion; and a changed DefaultShellCommandOption was not looked at at all. Both values are compared now, against what this run actually left -- which for the option is /c only where there had been a value that was not already /c. Verified on Linux against stubbed Windows cmdlets: a rule at 10.0.0.5 under a LocalSubnet request is left untouched and named, while an Any rule is still narrowed; a clean -Disable exits 0, one with -SkipKeys exits 0, and one whose firewall restore fails exits 1; and a deleted DefaultShell and a changed command option each stop the revert with the reason printed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1fb1a8b1f2

ℹ️ 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".

Thirteenth review round. Three findings, all consequences of the last two rounds' own fixes. A rule left alone was still recorded as ours. Last round taught the loop to skip a rule already inside the requested scope; the record was still written for it, one line above. A record is a claim of ownership and -Disable acts on it, so an untouched rule that an administrator later disabled would have been re-enabled from values this script never set. The entry now goes in only where an address, profile or enabled-state change is actually about to be made -- still before the first call that makes one, which is what round nine was for. Adding a key was the one write left that was not a swap. An append that failed part way could leave a complete, working key line in the file and still throw, and a key sshd accepts that the run never recorded is one -Disable never removes. The new contents are built in memory and swapped in, so either the line is there and on record or neither is. Existing bytes are copied across untouched, which is what the byte-preservation tests check. A record that cannot be deleted is emptied. The path where everything was restored deleted the file outright; a delete that failed left the full record behind, and a later -Disable would read it as entirely outstanding and put those values back over whatever had happened since. It is emptied instead -- a record claiming nothing is one a later run reads and correctly does nothing with -- and if even that fails, the run says so and exits non-zero. Writing the swap turned up one of its own, which the tests caught: an empty array returned from an `if` expression is unrolled to nothing, so [byte[]]@() in that position leaves $null and Array::Copy refuses it. Verified on Linux against stubbed Windows cmdlets: a rule already at the requested address is reported as left alone and does not appear in the record, while a wider one is still narrowed and recorded; a key added to a file with an existing key leaves both present, on record, and no temporary file behind; and a record whose deletion fails is emptied, leaving zero rules, zero keys and no managed shell for a later -Disable to act on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e4cb523d80

ℹ️ 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".

Fourteenth review round. Two findings. -Disable restored a competing firewall rule whatever had happened to it since. If an administrator disabled or further tightened one after setup, the original broad, enabled scope went back on -- at the very moment this script was removing its own narrow rule, so the machine came out of a "close remote access" run more open than it went in. Each record now carries what this run applied as well as what it found, and a rule is only put back where it still matches what was applied. Anything else belongs to whoever changed it: the run says so, leaves it, and drops it from the record rather than trying again later. This is the check I declined in round eleven. I judged it disproportionate for the case then in front of me -- a stale record after a failed partial disable, which needs three things to go wrong at once. This case needs one ordinary administrative action, and the outcome is worse. The earlier reasoning does not survive it. A login name with a space produced an unusable config. ssh_config splits an unquoted value on whitespace, so -User "Build Agent" was rejected before any connection was attempted. It is quoted now, along with HostName. The alias needed the opposite treatment, which is worth recording because quoting it looked like the obvious fix. Quoting the Host line does parse -- but ssh then refuses "my vm" as a destination outright, with "hostname contains invalid characters", before it reads any config at all. No quoting makes such an alias work, so -SshHostAlias now refuses whitespace up front rather than printing a config naming a host nothing can connect to. Verified: against real ssh 9.6, the emitted block with User "Build Agent" resolves to that user, and an alias with a space is refused by the script before it can be emitted. Against stubbed cmdlets, a rule an administrator disabled after setup is left alone while an unchanged one beside it is restored, and both are restored when neither was touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 06c5187822

ℹ️ 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".

Fifteenth review round. Four findings, all follow-ons from the last two rounds. A rerun that narrowed a rule further left the record describing the first run's result, so -Disable read this script's own second narrowing as somebody else's edit and abandoned the rule -- the round-14 check turned against the script. The record now keeps the ORIGINAL scope from whichever run first touched a rule (that is what -Disable puts back) while tracking what the LATEST run applied (that is what the rule should look like now). The same defect appeared a second way: a -Disable that restored a rule's address and then failed on its profile wrote back a record still claiming the old address, so the retry compared the rule against work it had itself already done and gave up. The kept record now advances a step at a time. Appending a key to a file whose last line had no terminator inserts a newline, and that newline was never removed. -Disable left the file one byte longer than it found it, every time, and there was nothing on record to say the byte was this script's. It is recorded now and comes back out with the key. Keys were matched by the whole line, comment included. Editing the comment by hand -- renaming a machine, say -- hid the key from -Disable, which then reported it removed while it was still an accepted login. Matching is on the type and the base64 blob, which is what sshd authenticates; the same change stops a rerun installing a second copy under a new comment. A key that really is not there now says so instead of claiming success. Verified against stubbed cmdlets: a rerun narrowing 10.0.0.5,10.0.0.6 to 10.0.0.5 still restores both rules to Any; a restore failing on the profile step is completed by the next -Disable; a file with no trailing newline is byte-exact after enable and disable, while one that had a newline keeps it; a key whose comment was edited is found and removed, and a key genuinely absent reports so. Each of the six was confirmed to fail against the previous commit. Two older suite cases had stopped exercising their claims -- the run now stops on an unnarrowable rule, and stubbed rule state leaked between cases -- and were repaired rather than left green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9ead5d5c2b

ℹ️ 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".

Sixteenth review round, and the point at which fixing the findings one at a time stopped being the right answer. Forty-nine findings over fifteen rounds, and the great majority of the last six rounds landed in one place: the rollback bookkeeping. Not one broken rule -- the same rule missing from whichever component nobody had yet written it into. Every change the script makes has to obey four rules. Record before mutating. Track what was applied rather than what was planned. Verify before reverting. Drop what is restored and keep what is not. Those were written out separately for the default shell, the sshd service, each competing firewall rule, each authorized-keys file and its DACL -- five record shapes, four hand-written comparisons, and the ACL had none at all. Adding a component meant reimplementing all four, and forgetting one was silent. That is what the review kept finding. There is now one ledger. An entry per change, with Original and Applied per field, and one implementation of each rule over all of them. A component supplies two things: how to read its current values, and how to write one of them. Restore order is declared by kind, and within one file the lower kind goes first -- which is what puts a key out before that file's terminator and both before the DACL protecting them, rules that were previously special cases in the disable flow. What that subsumes from this round's five findings: the enable path claiming a profile it never applied, the terminator trimmed from the wrong end of a file two runs had appended to, and the sshd service restored without checking -- all three are now the general rules applying to those components rather than three more patches. The remaining two were genuinely local and are fixed as such: the key identity now finds the key type rather than assuming it is the first field, so an entry hardened with `restrict` or `from="..."` is still recognised, and the printed ssh_config escapes backslashes and double quotes. The ACL gained verify-before-revert for free, which is the point. It was a known gap I had flagged and not closed; no code was written for it here. Net effect on the script is about 280 lines removed from the rollback paths and replaced by a smaller shared core. Six bespoke functions are gone: Merge-AdjustedRules, Test-RuleStillAsApplied, Disable-RemoteMcpFirewall, Restore-AuthorizedKeysAcl, Restore-SshService and Restore-SshDefaultShell. The on-disk record shape changed. Nothing reads the old one -- this script has not shipped -- so there is no migration; the file carries a Version now so a later change to it has somewhere to branch. Verified: all 28 stubbed suites pass, including every case written for rounds 2 through 16. Three behaviours are strictly better than before and the suites say so: a file with no trailing newline is now byte-exact after two enables and a disable rather than only after one; a restore writes only the fields that actually moved, so a retry after a partial failure completes the remainder instead of rewriting what it already did; and a key somebody else removed is detected and left rather than attempted. The suite also caught a bug in the ledger itself -- a [hashtable] parameter silently reorders an ordered dictionary, which made restore field order depend on hash bucketing. Still true, and still the thing worth a human's attention: the Windows-only half has never run on a real guest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 66cb048462

ℹ️ 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".

Seventeenth round, one P1, and it is in the ledger I added last commit -- the rule was right and the merge undermined it. Merge-ChangeLedger replaced the stored Applied map wholesale with the rerun's. But a run records an Applied value for EVERY field of a change, not only the ones it wrote: Applied starts equal to Original, which is whatever the machine held when that run looked. So a field the rerun merely observed went into the record as this script's own work. Rule 3 then found no drift and put the pre-enable value back over it. The reported case is the bad one. An administrator narrows a competing rule's profile to Private after setup; a rerun that only re-narrows the address records Private as though this script had applied it; -Disable sees no drift and writes the original Any back -- re-widening to Domain, Private and Public a rule somebody deliberately tightened, during the run whose whole purpose is closing access. Confirmed by reproduction, and the same defect reaches the default shell: an administrator setting DefaultShellCommandOption to /c had it deleted by the next -Disable. The merge is now field by field. A field the rerun wrote is one whose latest Applied differs from its latest Original, because Applied only advances on a write that returned; everything else keeps what the earlier run recorded, so drift against that earlier value is still detected. The stored side is rehydrated before merging, since assigning per field needs something indexable rather than what ConvertFrom-Json returns. Also fixed a harness gap that hid half of this: the firewall stub's pipeline branch returned each rule's original profile and enabled state, ignoring changes made through Set-NetFirewallRule, so a test about an administrator's edit was quietly testing nothing. The firewall half of this finding did not reproduce until that was corrected -- worth recording, because the first run of the new test looked like evidence the report was wrong. Verified: 29 suites pass. A rerun after an admin narrows the profile leaves applied at Domain,Private and -Disable leaves the rule alone; a rerun that really does re-narrow the address still merges that and is still restored; and an admin's DefaultShellCommandOption survives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 752f7ac211

ℹ️ 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".

…ponents Eighteenth round: one P1 and one P2, both in the ledger, both about treating a change as indivisible when it is not. Restore order was wrong for a run that closes access. FirewallRule went first, so -Disable widened a competing rule back to the machine's original scope while the key it installed was still in the file and sshd was still listening -- a window where the machine is more open than at any point while remote access was deliberately on. Order is now: stop the service, take the key out, tidy the file, hand the DACL back, then the shell, then the firewall last. Everything that removes access happens before anything that restores somebody else's broader configuration. The two orderings that were already load-bearing -- a key before its file's terminator, both before the DACL -- are unchanged, since they are relative and within a scope. That has a cost worth stating: sshd now stops first, so a -Disable run over the very ssh access it is closing loses its session part way through. This script lives on the guest and is run there; the record it leaves is safe to retry either way, because a field already restored equals its Original and is skipped. Ownership was per component when it is really per field. Any drift in any field made -Disable disown the whole entry. An administrator who set sshd's startup type to Disabled left Running untouched, so the service this run started was abandoned STILL RUNNING, for ever -- with the firewall already widened, under the previous order, to a live password-authenticating sshd. The same flaw left this script's own narrowing stranded on a firewall rule it had just disowned and dropped from the record, so nothing would ever undo it. Ownership is now per field: drifted fields are left to whoever changed them, undrifted ones are restored, and the run says which is which. The default shell opts out with Coupled -- DefaultShell and DefaultShellCommandOption have to agree, since sshd runs `<shell> <option>` and cmd wants /c while pwsh wants -Command, so restoring one over an administrator's other half produces a combination neither party chose. That keeps the round-12 decision intact rather than quietly reversing it. "(gone)" is not a field and never was; it is a named sentinel now, and it disowns the whole entry as before. Two bugs of my own found while writing this, both by the suites: - the new parameter was called $Field, which PowerShell clobbers with the $field loop variable inside the same function, since variable names are case-insensitive. It is $Owned. - [bool](Get-ChangeAdapter ...).Coupled casts the adapter, not the property. Verified: 30 suites pass. -Disable now stops sshd and restores the DACL before the first widening; an admin's Disabled survives while the service is still stopped; an admin's profile change survives while the address this script narrowed is still given back; and the shell is still all-or-nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 63a3ebe9be

ℹ️ 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".

…e thing Nineteenth round: one P1 and two P2s. The P1 reverses part of round eighteen, which is worth saying plainly rather than burying. A firewall rule's address, profile and enabled flag are one scope, not three independent settings. Round eighteen made ownership per field, which is right for the sshd service -- an administrator's Disabled must survive while the service this script started is still stopped -- and wrong here. An administrator who moves a narrowed rule from Private to Public, leaving the address alone, got the address widened back to Any underneath them: Any/Public, broader on that profile than the machine ever was, and a combination neither party configured. FirewallRule is Coupled now, like the default shell. A rule left narrower than this script found it is the safe failure; the run says it is leaving it. Round eighteen's own test case for the old behaviour is rewritten rather than deleted, with the reversal and the reason in it. The key file's terminator was recorded as "the last byte of the file" when it is really a byte at a position. Once anything else appends a terminated line, the last byte belongs to that line, and removing it joined two lines that were never joined -- the opposite of the byte-for-byte preservation this promises. The entry now carries the offset the terminator went to, and is this script's to remove only while the byte it wrote is still there AND still ends the file. Anything after it is reported as changed and left: one newline more than the file started with, which sshd does not mind, against corrupting a line this script did not write. Enable started sshd before narrowing the firewall. The capability's own exception covers Domain and Private, and any exception already on the machine covers whatever it covers, so there was a window -- short, but real -- in which the listener answered from outside -ClientAddress, where pre-existing keys and password authentication both work. The firewall step needs nothing from a running sshd, so it goes first; a rule that cannot be narrowed now fails the run before anything is listening at all. Five steps rather than four, since installing the capability and starting the service are no longer adjacent. Verified: 31 suites pass. The rule an administrator moved to Public is left entirely alone; an externally appended line keeps its own newline while the original bytes stay intact; and narrowing is logged before service-started rather than after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ca86b30730

ℹ️ 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".

…t scope Twentieth round, two P2s, both about a window rather than a wrong value. The ledger only reached disk in the finally. Rule 1 says record before mutating, and it did -- in memory, which does not survive what actually interrupts a run. A terminated process left the machine changed with nothing written down; the worst case is a key installed and unrecorded, which is a live credential -Disable will never find. Confirmed by instrumenting the stubs to read the on-disk record at the moment of each mutation: every one of them saw zero entries. The ledger now writes itself whenever it changes, which is exactly two functions -- New-ManagedChange and Set-ChangeApplied -- so a component added later gets this without knowing it exists, same as the other three rules. Writes are whole-record and atomic, so a kill during one costs the update rather than the file, and the record is behind the machine by at most the call in flight. The same instrumentation now reads 1, 2, 3, 5. A side effect worth having: a run that cannot write the record now fails at the first change it cannot record instead of making all of them and failing at the end. The explanatory message came back with it -- the first cut of this surfaced a raw WriteAllBytes exception, which is worse than what it replaced. This script's own rule was rescoped in place while enabled. Profile, port and address are three separate calls, so a rerun moving from client A on Private to client B on Public allowed A on Public in between -- brief, but on a rerun sshd is already running and answering. It is disabled first and re-enabled last, so every intermediate state is closed rather than open. Two harness gaps fixed to get there, both of which had been hiding this code from its own tests: the stubbed Get-NetFirewallRule never let this script's own rule exist after creation, so no suite had ever taken the rerun path through it, and Get-NetFirewallPortFilter did not accept a rule from the pipeline. Verified: 32 suites pass. The on-disk record precedes every mutation; the rerun logs en=False, port filter, address filter, en=True in that order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LHeVmMkveihytyfLLxp7Nk

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0bb33d2953

ℹ️ 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".

chatgpt-codex-connector · 2026-08-21T12:15:43Z

+    [System.Array]::Copy($existingBytes, 0, $combined, 0, $existingBytes.Length)
+    [System.Array]::Copy($addition, 0, $combined, $existingBytes.Length, $addition.Length)
+
+    Set-FileContentAtomically -Path $Path -Bytes $combined


Make interrupted applied-state updates recoverable

If the VM or PowerShell process stops after this atomic replacement but before Set-ChangeApplied persists Present=True, the durable ledger still says the key is absent even though sshd can use it. A later -Disable compares the live Present=True with the recorded applied value, classifies the key as external drift, drops the entry, and leaves the credential installed. Fresh evidence beyond the earlier write-ahead finding is that the new pre-write entry is durable, but it contains no pending/in-flight state that recovery recognizes; persist intent that -Disable treats as removable before the replacement, or otherwise reconcile interrupted mutations.

AGENTS.md reference: AGENTS.md:L27-L29

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-21T12:15:43Z

+        if ($priorAddress.Count -eq 1 -and "$($priorAddress[0])" -eq "Any") {
+            $narrowedAddress = $RemoteAddress
+        } elseif ($addressWithinRequest) {
+            # Already inside the request: nothing to do, and writing the


Treat a requested Any address as universal

When the caller explicitly uses -ClientAddress Any, an existing rule scoped to a concrete address or LocalSubnet is necessarily already narrower, but this branch tests only whether the existing rule itself is Any; the subsequent exact-overlap check finds no match and marks the scope incomparable. Consequently a valid broad request aborts whenever such a narrower competing SSH rule exists, even though that rule cannot widen the requested boundary. Handle RemoteAddress containing Any as an already-contained case before the exact-membership logic.

Useful? React with 👍 / 👎.

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1

Comment thread scripts/Setup-RemoteMcp.ps1

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

glslang and others added 2 commits August 21, 2026 08:16

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread scripts/Setup-RemoteMcp.ps1

Comment thread scripts/Setup-RemoteMcp.ps1 Outdated

glslang merged commit 30c49d1 into main Aug 21, 2026
3 checks passed

glslang deleted the claude/agent-sandbox-vm-setup-ubpye7 branch August 21, 2026 12:14

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

Conversation

glslang commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What it configures, and why each one is scripted rather than described

Reversal: one ledger, four rules

Affected host platform

Validation

VM lifecycle impact

Review history

Uh oh!

coderabbitai Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

glslang commented Aug 21, 2026 •

edited

Loading

coderabbitai Bot commented Aug 21, 2026 •

edited

Loading