Add Windows warm daemon IPC - #641
Conversation
|
Quick note on the benchmark workflow change in The first CI run completed the benchmark comparison successfully, but failed afterward when the workflow tried to post The compare step still fails the job for real benchmark regressions. This change only makes the final reporting comment best-effort when GitHub does not grant the fork PR token permission to create comments. The benchmark report is still available in the GitHub Actions run under the |
|
@justrach this is ready for review now. I targeted |
![chatgpt-codex-connector[bot]](https://cdn.github.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: 6541fad91c
ℹ️ 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".
justrach
left a comment
justrach
left a comment
There was a problem hiding this comment.
Thanks for picking up the named-pipe IPC follow-up @nsxdavid — the MSVCRT argv quoting + round-trip test and the response-size cap are genuinely clean. Before it lands I'd like to tighten the trust model on a couple of the newly-enabled Windows paths. Two main, two minor.
1. Named-pipe squatting → spoofed CLI output + daemon DoS (Medium)
The pipe name is predictable (\\.\pipe\codedb-{Wyhash(0xc0de, root)}-{Wyhash(0xc0de, %USERNAME%)}) and the listener closes + re-creates the instance every loop. A local process can pre-create that name (or win the close/re-create gap):
- our
CreateNamedPipeAthen fails and the handler doesshutdown.store(true); return;→ daemon dies (one-shot DoS); - the client opens the name with
OPEN_EXISTING, never checks who owns the server end, and writes the reply straight to the terminal (cio.File.stdout().writeAll). A squatter receives the request blob (project path + argv) and returns arbitrary bytes → spoofed output + ANSI-escape injection.
SECURITY_SQOS_PRESENT | SECURITY_IDENTIFICATION on the client is the right call (stops a malicious server impersonating us) but doesn't authenticate the server to the client. Suggested:
- Client verifies server:
GetNamedPipeServerProcessId→ compare that PID's token-user SID to our own; bail on mismatch. (Main fix — defeats squat + instance-hijack regardless of name predictability.) - Don't
shutdownon a singleCreateNamedPipeAfailure; log + retry. - Close the re-create gap (pre-create the next instance before closing the current, or overlapped accept).
2. runCapture runs unqualified git in the repo cwd → potential Windows binary-planting (High, confirming)
runCaptureWindows now actually runs getGitHead's git rev-parse HEAD with argv[0]="git" and cwd = <repo root>. If Windows resolution searches the cwd (incl. PATHEXT), a git.exe/git.bat planted in a repo executes when codedb tags the snapshot SHA — which is automatic. I'm reproducing this on a Windows box; if it confirms, the fix is to resolve git to an absolute path (PATH search excluding cwd) before spawning. Same care for the other unqualified execs.
3. self-update re-enabled on Windows (Low) — runCapture also un-gates download-and-replace; just confirm the Windows update path is TLS + checksum/signature verified.
4. pipe DACL (Low) — D:P(A;;GA;;;OW) keys on Owner Rights; for an elevated process the owner can default to Administrators, and there's no integrity label. Consider pinning the ACE to the current user's token SID and adding S:(ML;;NW;;;ME).
Happy to pair on the client-side SID check — that one change closes most of the surface. Thanks again!
|
Confirmed F2 (binary-planting → code execution) on a real Windows box — and it's a source-level guarantee, not luck. Empirical PoC (Windows Server,
So indexing a repo that merely contains a Why (Zig 0.16.0 // "The cwd provided by options is in effect when choosing the executable path to match POSIX semantics."
const cwd_path_w = ... else if (cwd_w) |cwd| break :x cwd; // child cwd = repo root
var dir_buf: std.ArrayList(u16) = .empty;
if (cwd_path_w.len > 0) try dir_buf.appendSlice(arena, cwd_path_w); // dir_buf seeded with cwd
windowsCreateProcessPathExt(arena, &dir_buf, &app_buf, PATHEXT, ...) // searched FIRST, before PATHSo even on a box with real git installed, the planted copy wins (cwd is first in the search). Fix: resolve |
|
@justrach working the identified issues |
6541fad to
0dc0882
Compare
|
@justrach I updated this PR with the Windows follow-up fixes for the review feedback. What changed:
I intentionally did not change self-update behavior in this PR. That seemed out of scope for the IPC hardening work; if you want that tightened, I think it should be handled as a separate, explicit follow-up. Validation:
|
|
@codex can you go thru the pr thanks! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dc088214f
ℹ️ 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 Don't know if I am authorized to command you, but on the off chance that I am, please review the latest changes and responses to your previous review comments |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60e3ea93fd
ℹ️ 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".
|
@justrach this appears ready for merge from my side. The latest head is \cbd76d3, the benchmark check is passing, and the addressed review threads have been replied to and resolved. |
Resolves the commands.zig conflict between justrach#645 (.codedbrc disable gating in runMcp) and this branch's data_dir threading for the Windows pipe metadata: the cliDaemonListen spawn keeps the new data_dir argument and stays inside the !project_disabled block, alongside spawnWarmup.
|
Heads-up @nsxdavid: three fixes landed on Validation on the merged tree (macOS): |
…5828 justrach#645 was reverted upstream (37aa83e) — maintainer call, auto-indexing stays core behavior. commands.zig resolves back to this branch's original runMcp (data_dir threading, no project_disabled gating).
|
Correction to my last note: #645 (.codedbrc disable) was reverted upstream (maintainer call — auto-indexing stays core behavior), so I pushed one more merge commit (a48678d) bringing your branch in line: Re-validated after the fixup: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dc646f705
ℹ️ 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".
| const metadata_path = std.fmt.allocPrint(allocator, "{s}/{s}/cli-daemon.pipe", .{ projects_dir, entry.name }) catch continue; | ||
| defer allocator.free(metadata_path); | ||
| const metadata = std.Io.Dir.cwd().readFileAlloc(io, metadata_path, allocator, .limited(1024)) catch continue; | ||
| defer allocator.free(metadata); | ||
| const pid = parseWindowsDaemonPid(metadata) orelse continue; |
There was a problem hiding this comment.
When multiple warm daemons exist for the same project, e.g. a serve and an mcp process both running with retry=true, they share this single cli-daemon.pipe record and the last publisher is the only PID this loop sees. nuke then kills just that process and immediately proceeds to deleteTree, while the older daemon can still be alive holding project data files or mmaps open on Windows, so the removal can still fail or leave a daemon behind. Consider repeating the metadata scan/kill until no owner republishes, or tracking all matching codedb warm-daemon PIDs.
Useful? React with 👍 / 👎.
|
Full validation is done — merging. Summary for the record: Windows (real VM, clean environment — Daytona windows-medium, 2 vCPU/8GB, SYSTEM account):
Non-Windows regression review (independent deep review + local runs):
Thanks @nsxdavid — this is quality work, and it ships in 0.2.5828. |
) appendOne kept its K-bounded candidate buffer sorted by re-running std.mem.sort on every accepted insert — O(K log K) compares per insert, plus a redundant final sort of the already-sorted list. The list is sorted at all times, so place each accepted candidate by binary search + ArrayList.insert (O(log K) compares + O(K) moves) and drop both sorts. The comparator also gains line_start as a final tiebreak: same-name symbols in the same file compared equal, so their relative order still fell out of hash-map iteration — the residual nondeterminism the deterministic-ordering rewrite (#641) meant to eliminate. Gated bench (same-session A/B, quiet-machine controlled via stash test): codedb_symbol 23.1us -> 19.0us (-18%); payloads byte-identical across exact/prefix/glob/fuzzy/kind/many-match query shapes; all other tools unchanged. zig build test 23/23 steps, 869/873 (4 platform skips). Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…#649) appendOne kept its K-bounded candidate buffer sorted by re-sorting the whole buffer on every accepted insert (O(K log K) compares per insert, plus a redundant final sort). The buffer is always sorted, so placement is a binary search + insert. The comparator also gains line_start as the final tiebreak: same-name same-file symbols compared equal, so their relative order fell out of hash-map iteration order — the residual nondeterminism the deterministic-ordering rewrite (#641) meant to eliminate. With the key ending in (path, line) and same-(path, line) deduped, ties are impossible and the sorted-insert invariant is total. A/B on this repo (exact/prefix/pattern/fuzzy/kind × max_results 200): payload bytes identical on every query; warm prefix query 3.1ms -> 896µs, name query 1.2ms -> 931µs (embedded dispatch timings). Gated bench: codedb_symbol +0.23% on the 21-file corpus (exact-name query barely exercises the insert path — the win scales with match count). zig build test: 23/23 steps, 869/873 (4 platform skips), incl. a new ordering test. e2e_mcp_test.py: 20/20. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
Refs #621.
Validation
zig build --global-cache-dir .zig-global-cache test --summary all23/23 steps succeeded; 858/862 tests passed (4 skipped)zig build --global-cache-dir .zig-global-cache --summary all3/3 steps succeededPYTHONIOENCODING=utf-8 python scripts/e2e_mcp_test.py --binary zig-out/bin/codedb.exe --project .20/20 passed