[malpern]

Adds ADR-046, the review #1354 asked for, and points ADR-013 at it. Docs only, plus a doc comment in KanataTCPClient.swift. No runtime code changes. Refs #1354; it does not close it: the fix and the release gate are follow-ups once the ADR is accepted.

Attack surface today

The session worker binds 127.0.0.1:37001 with no authentication. Loopback TCP isn't limited to one user or kept out by the App Sandbox. That means any local process can connect, including another logged-in user's processes and sandboxed apps with network.client. A connection joins the broadcast set before it sends a byte. From the fork at d9085a9, the commit master pins, which is on keypath/bundled (tip 54cdba9 merges it with an identical tree):

• Keystroke stream: every physical press, release and repeat is broadcast as KeyInput to every client.
• Config read: RequestCurrentLayerInfo returns the config text. Virtual-key and layer names are readable too.
• Input injection: ActOnFakeKey presses, taps or toggles any virtual key, so a client can trigger key output and macros. ChangeLayer switches layers. SetMouse moves the pointer.
• Config replacement: ReloadFile { path } loads any file the worker can read as the active config.
• Reaching KeyPath's action dispatcher: a virtual key bound to push-msg produces a MessagePush. KanataEventListener passes it to ActionDispatcher, which can launch apps, open URLs, run system and runtime/stop actions, and run scripts if the user enabled them.
• Port squatting: whoever binds the port first wins. The worker's passthru path panics on bind, and KeyPath's clients then trust the squatter's MessagePush.
• DoS: each connection gets its own thread, with no limit. Notification writes block while the client lock is held, so a client that stops reading can stall events for everyone (read from the code, not tested).

cmd is compiled out, so the engine itself cannot run shell commands. KeyPath never uses ReloadFile, ReloadNext/Prev/Num or SetMouse.

Recommendation

Move engine control to an owner-checked Unix socket. It would use the same directory and getpeereid pattern as RuntimeControlServer, plus a peer-PID check (only the app that launched this worker may connect) and later a code-signature check. Release builds would open no TCP port at all. In the same fork change, drop the unused commands, cap connections and add write timeouts. The CLI's direct TCP use (layer current/switch, service reload) would go through the app's control socket. A per-launch token over TCP was considered and rejected: it doesn't stop port squatting and it adds token-handling code. Narrowing the commands alone was also rejected: KeyPath needs KeyInput and ActOnFakeKey, so the keystroke stream and injection would stay. The ADR estimates the cost as roughly 300–500 lines in the fork, a bridge FFI change, two Swift transport sites, and changes to verify-installed-app.sh and the docs.

Follow-ups (not in this PR)

• Implementing the ADR.
• A release-process check that fails while release-blocker issues are open. Nothing enforces that today.
• Rewriting the TCP section of guides/privacy.md once the implementation lands.
• A separate review of the keypath:// URL scheme as a local entry point.

Verification

• Every claim in the ADR comes from reading the fork and KeyPath source at the commits named in the ADR. Nothing was tested live: KeyPath isn't installed on this Air and the mini is offline.
• ./Scripts/review-gate.sh ran locally: PASS. It had five minor notes. Four of them (wording in ADR-013's body, the passthru bind panic and probe race, the context-integration design that depends on the port, and the description of the privacy guide) are fixed in the second commit. The fifth note was cosmetic wording in the doc comment; I fixed that too.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-05T04:19:23.072084Z	96cf1b2	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Review: docs-only change (ADR-046 + comment/ADR-013 updates). No blocking findings.

Minor, non-blocking notes:

• ADR-046 Option C relies on LOCAL_PEERPID for the app-PID check. PIDs can be reused and the check alone is weaker than the LOCAL_PEERTOKEN audit-token signature check. Since the signature check is deferred to phase 4 while the release gate is phases 1-3, consider stating explicitly that attacker 3 (same-user, unsandboxed) is only partly mitigated at release time (a same-user process could still spawn or inject into a process matching the PID, though that is arguably out of scope).
• The ADR says the release gate for Review the unauthenticated TCP endpoint for the session runtime #1354 is not enforced by release-doctor.sh. Consider filing a follow-up so the blocker doesn't depend on memory.
• The ADR states facts were read from the fork and not exercised on a live install; a quick live confirmation (e.g. connecting from a second user or sandboxed app and observing KeyInput) would strengthen the claims before the ADR is accepted.
• The KanataTCPClient.swift change is comment-only, so there is no behavioral or test impact.

Index/README status entries are consistent with the ADR-013 header update.

[claude]

Review: docs-only ADR + comment change; no blocking findings.

The Swift change is a comment edit only, so there is no runtime risk. A few points on the ADR:

1. Unverified claims. The ADR says it was written from source and that the main claims (another uid or a sandboxed app receiving KeyInput) were not tested on a live install. "Proposed" status fits. Keep Review the unauthenticated TCP endpoint for the session runtime #1354 open until the lab-VM confirmation is done.
2. Attacker 3 gap. Option C only stops same-user, unsandboxed processes (attacker 3) once the phase 4 signature check ships. The ADR defers phase 4 to a point release. Until then a uid and PID check leaves attacker 3 with PID-spoofing or injection avenues. The ADR says it treats these as out of scope, but "closes every case in the threat model" overstates it. Consider rewording that bullet.
3. PID reuse. The LOCAL_PEERPID check relies on the worker exiting when the app does. Spell out how that is enforced, for example parent-death detection. Otherwise a reused PID can pass the check.
4. Doc consistency. The ADR-013 header, README and index all say "Under review, see ADR-046". The ADR-013 status line says "to be superseded". This is consistent enough, but the README and index labels could say "Proposed to be superseded" so they match.
5. Minor. The KanataTCPClient.swift security note mentions "ADR-046 / Review the unauthenticated TCP endpoint for the session runtime #1354", which is fine. It will need updating when the migration lands (the ADR already lists this under Docs).

No test-coverage concerns, since nothing executable changed.

🤖 Generated with Claude Code

[claude]

Review: docs-only ADR plus a doc-comment change. No code behavior changes, so there are no blocking findings.

Notes for the author:

1. Unverified claims. The ADR says the claims were "read from source" and not exercised on a live install. The proposed status and the "confirm in lab VM before accepting" step handle this. Please run that check before the ADR moves to Accepted. It should cover a second macOS user and a sandboxed app, each connecting and receiving KeyInput.
2. Option C, PID check. The PID-reuse reasoning is sound. Phase 1 also needs the worker to exit when the owner dies (the kqueue NOTE_EXIT source you describe). Without it, LOCAL_PEERPID is only a weak check. Consider making that source a stated acceptance criterion for phase 1.
3. Phase ordering. Phases 1 to 3 close attackers 1, 2 and 4 and stop direct connections from 3. Phase 4 (the signature check) is what stops a same-user process from impersonating the app. The ADR says this in several places, which is good. A single summary table of attacker versus phase would make it easier to read.
4. Compatibility. Dropping localhost:37001 scripting is called out for release notes. Add a checklist item in the release process so the note doesn't get lost.
5. Comment accuracy. In KanataTCPClient.swift, the new security note describes ReloadFile and the keystroke stream as present facts. That is fine while Review the unauthenticated TCP endpoint for the session runtime #1354 is open. Update or remove it when ADR-046 lands.
6. Index files. README.md and index.md are consistent with each other. The ADR-013 inline annotations also read correctly.

No test or code changes are needed for this PR.

[claude]

Review: docs-only ADR + a comment update in KanataTCPClient.swift. No code behavior changes, so no correctness or test-coverage risk. A few notes on the ADR:

1. Unverified claims. The ADR says it was written from source only (and the "confirm on a live install" step is listed as a precondition to acceptance). That's the right framing. But the KanataTCPClient.swift comment and the ADR-013 note state these attacks as fact ("Any local process, including other users' processes and sandboxed apps ... can connect ... receive every KeyInput"). Consider softening the code comment until the lab VM check is done, or noting "per source reading".
2. Attacker 3 / phase 1-3 residual risk. The decision table says a same-user unsandboxed process is "direct connection blocked (PID check)". LOCAL_PEERPID only identifies the peer process; any same-user process can't spoof its own PID, so this holds, but the ADR should state explicitly that the PID check compares against the app's PID (not just "any process owned by the user"), and that the check must happen on the accepted fd, not via a later lookup. It reads that way already; a one-line clarification would help.
3. ADR-013 status wording differs slightly between adr-013 ("Under review; to be superseded") and README/index ("proposed to be superseded"). Harmless, but worth aligning.
4. Date consistency. ADR-013 note is dated 2026-11-04 and replaces the earlier 2026-10-31 note; fine, just confirm the original ADR-045 pointer isn't lost (it's dropped from the new note text).
5. Release gating. The ADR notes nothing enforces the release-blocker check in release-doctor.sh. Worth a follow-up issue so Review the unauthenticated TCP endpoint for the session runtime #1354 can't ship accidentally.

No blocking findings.

[claude]

Review: docs/ADR-only change (plus a doc-comment edit in KanataTCPClient.swift). No code behavior changes, so no runtime risk.

Findings (all minor, non-blocking):

1. ADR-046 phase numbering is inconsistent. The Migration section lists steps 1–5 (fork, bridge/worker, Swift clients, signature check, docs), but the Decision says release ships after "phases 1 to 3" and phase 4 is the signature check, and the Compatibility paragraph refers to "Phase 5" for release notes. Release notes are in step 5 (Docs), so this works, but the word 'phase' is never defined in the Migration list. Consider labelling the steps 'Phase N' explicitly.
2. PID-check weakness is acknowledged but the release gate relies on it. The PID reuse window (5 ms tick plus isProcessAlive) is covered by the kqueue NOTE_EXIT acceptance criterion. It would help to also list it as a required test (kill the app, assert the socket stops accepting) so it is not dropped when phase 1 is implemented.
3. Unverified claims. The ADR states the other-user and sandboxed-app attacks are from source reading only. The KanataTCPClient doc comment mirrors that caveat, which is good. Consider linking the lab-VM verification as a tracked item on Review the unauthenticated TCP endpoint for the session runtime #1354 so ADR acceptance has a concrete checkbox.
4. Doc comment placement. The new SECURITY NOTE is accurate to the ADR; just keep it in sync when ADR-046 is accepted or changed, since it names specific commands (ReloadFile, ActOnFakeKey).

No correctness, security, or test-coverage issues in the diff itself. ADR README/index entries are consistent with each other.

[claude]

Review: docs-only ADR + comment change. No blocking findings. The Swift change only edits a doc comment, so there's no runtime risk.

A few minor, non-blocking notes:

1. Option C, PID check and Phase 1/3 split. The ADR relies on LOCAL_PEERPID matching the owner PID for attacker 3. It correctly notes the PID-reuse window and requires a kqueue NOTE_EXIT plus a re-check on every accept. Because that is an acceptance criterion, put it in the Review the unauthenticated TCP endpoint for the session runtime #1354 checklist too so it isn't lost when the ADR is accepted.
2. Release-blocker enforcement. The ADR says nothing enforces open release-blocker issues (release-doctor.sh doesn't look). Consider a follow-up issue so Review the unauthenticated TCP endpoint for the session runtime #1354 is not just a convention.
3. Unverified claims. The ADR says its claims come from source reading and have not been confirmed on a live install. The KanataTCPClient comment hedges the same way, which is appropriate. Run the lab-VM confirmation (second user, sandboxed app) before moving the status from Proposed to Accepted.
4. Interim mitigation. Until phases 1-3 land, the push-msg -> ActionDispatcher path is the highest-impact item that can be fixed on the Swift side alone, for example confirmation for script, runtime/stop and non-http open when the URI comes from the engine. The ADR mentions this under Consequences; consider tracking it as its own issue.
5. Wording. In adr-013, the status line says "Under review; proposed to be superseded ... (Proposed)". The trailing "(Proposed)" is redundant.

Otherwise LGTM: the index/README entries are consistent with the ADR status.

[claude]

Review: docs-only ADR plus a comment change in KanataTCPClient.swift. No blocking findings.

Minor, non-blocking notes:

1. Unverified claims. The ADR says its claims come from source reading and not from a live install. The KanataTCPClient.swift comment states them as fact ("any local process, including other users' processes and sandboxed apps … can connect… receive every KeyInput"), with only a brief "from reading the source" hedge. That is fine for a release-blocker flag. Consider linking the lab-VM confirmation task from Review the unauthenticated TCP endpoint for the session runtime #1354 so the hedge can be dropped once it is confirmed.
2. Phase 1 PID check. LOCAL_PEERPID is checked at accept time. The ADR already notes the PID-reuse window and mitigates it with kqueue NOTE_EXIT plus an owner re-check on every accept. Phase 1 should also state that the check applies to the process that launched the worker. If the app relaunches the worker, the expected PID must be passed again on each launch.
3. Attacker 4 and option C. The 'bind once and keep the listener' fix is right. For the Unix socket, also note that the socket path length is limited to about 104 bytes on macOS (sun_path). A per-user directory under a long $TMPDIR or Application Support path can exceed that limit, so choose the location with this in mind.
4. ADR-013 status wording. "Under review; proposed to be superseded by ADR-046 (Proposed)" is accurate. The docs/adr/index.md and README rows are consistent with it.

The ADR and doc edits are consistent with each other, and no code behavior changes.

[claude]

Review: docs-only ADR plus a doc-comment change, with no code behavior changes. No blocking findings.

Minor, non-blocking notes:

1. Phase ordering and the PID check. ADR-046 relies on LOCAL_PEERPID plus owner-liveness for attacker 3. The doc already says the PID check is weak against PID reuse, and it requires a kqueue NOTE_EXIT watch and a re-check on every accept. Consider also requiring the worker to exit, not only stop listening, when the owner dies. The test sketch covers this ("worker exits"), so this is just a reminder to keep it as a hard acceptance criterion.
2. Phase 4 versus release. The table says attacker 3 is "Direct connection blocked (PID check)" after phases 1–3. A same-user process can still connect by spawning or injecting into the app. The ADR notes this as out of scope, which is fine, but the "Blocked" wording in the table could say "partially" so readers don't over-trust it.
3. Unverified claims. The ADR states that it was written from source only. The cross-user and sandboxed-app claims are the basis for the release-blocker status, so run the lab-VM confirmation before moving the status from Proposed to Accepted.
4. Doc-comment wording. The KanataTCPClient.swift comment says "From reading the source (not yet confirmed on a live install)". Remember to update or remove that hedge once the lab confirmation is done, so the code comment doesn't go stale.
5. Date consistency. The ADR-013 note is dated 2026-11-04, matching ADR-046, and the existing note was 2026-10-31. That is consistent.

README and index entries match each other. LGTM from a correctness standpoint.

[claude]

Review: docs-only ADR plus a comment edit. No blocking findings.

Minor, non-blocking notes:

1. Phase 1 PID check is shaky. The ADR already admits the dead-PID reuse window. Make the kqueue NOTE_EXIT watch and the per-accept liveness re-check hard acceptance criteria. The ADR does this already. Keep the required kill-owner test in the phase-1 checklist.
2. Unverified claims. The comment in KanataTCPClient.swift and the ADR-013 note state the exposure (keystroke stream, ReloadFile, sandboxed-app access) as fact. The ADR says these were read from source and not run on a live install. The Swift comment does say 'from reading the source', which is good. The ADR-013 note drops that caveat. Consider adding it there too.
3. Option C scope gap. keypath:// URL entry points stay unauthenticated, so attacker 3 keeps an indirect path to the engine's ActionDispatcher and virtual keys. The ADR flags this as 'Related, not decided here'. Link it to a tracking issue so it isn't lost.
4. Release-blocker enforcement. The ADR notes that nothing enforces open release-blocker issues. Consider a follow-up issue for a release-doctor.sh check.

The Swift change is comment-only, so there is no behavior or test impact.

[claude]

Review: no blocking findings. This PR changes only docs and a doc comment in KanataTCPClient.swift, so there is no runtime risk.

Notes on ADR-046:

1. Unverified claims. The ADR says its claims come from source reading only. It already requires a live-VM check (second user, sandboxed app) before acceptance. Please keep that gating.
2. Phase 1 PID check. LOCAL_PEERPID on the accepted socket is fine. The kqueue NOTE_EXIT plus per-accept liveness re-check is the right mitigation for PID reuse. The kill-the-owner test is a good acceptance criterion.
3. sun_path limit. ~/Library/Application Support/KeyPath/control/ can exceed 104 bytes for long home paths. The ADR says to fail with a clear error. Consider also specifying a fallback, such as a short path under $TMPDIR or a per-user /var/folders directory, so those users aren't locked out.
4. Release-blocker enforcement. The ADR notes that nothing enforces Review the unauthenticated TCP endpoint for the session runtime #1354 as a release-blocker. Consider a follow-up issue to add an open-release-blocker check to release-doctor.sh.

Docs and index updates (ADR-013 note, README, index) are consistent.

[claude]

Review: docs/comment-only PR, no blocking findings.

The Swift change only rewrites a doc comment in KanataTCPClient.swift. The rest is ADR text and index updates. A few notes:

1. Status inconsistency. ADR-013 now says "Under review; proposed to be superseded", which matches ADR-046 being Proposed. But AGENTS.md and the ADR-013 body still say "Accepted"-era things, such as the "Same security model as Unix sockets" line. You've annotated that line, which is good. Check that no other doc (for example guides/privacy.md, docs/architecture/overview.md) still calls the risk low. ADR-046 Phase 5 already lists these.
2. Unverified claims are shipped as facts in a code comment. The KanataTCPClient.swift comment says "any local process, including other users' processes and sandboxed apps ... can connect". It does hedge with "from reading the source (not yet confirmed on a live install)". Once the lab VM check is done, update or drop that hedge. Otherwise it will go stale.
3. ADR-046 Option C detail. PID-based peer checks (LOCAL_PEERPID) are weak against PID reuse. The ADR addresses this with kqueue NOTE_EXIT plus a liveness re-check on accept, and makes both an acceptance criterion. That is good. It would help to also state that LOCAL_PEERPID is read at accept time and is still racy against a fork+exec by the owner. The signature check in phase 4 is the real fix, which the ADR already implies.
4. Release-blocker enforcement. The ADR notes that nothing enforces open release-blocker issues. Consider filing a follow-up issue so that gap is tracked and doesn't live only in the ADR prose.

No code, test, or build risk from this diff.

[claude]

Review: docs-only change plus a comment edit in KanataTCPClient.swift. No functional code is touched, and I found no blocking issues.

Minor, non-blocking notes:

1. Phase 1 and the passthru path. The ADR says the PID check needs LOCAL_PEERPID and that ownerPID is already passed to the worker. It also says the expected PID must be re-passed on every launch. Before accepting, spell out what happens if the app restarts and adopts a still-running worker. As written, the worker would exit via the NOTE_EXIT watch, so adoption is impossible. Say so explicitly, because the current verify and adopt flows may assume reuse.
2. LOCAL_PEERPID caveat. The ADR already notes that fd-passing to a child defeats the PID check. That is a fair note. Consider also stating that LOCAL_PEERPID is a socket option read at accept time, which is correct for this design. The 5 ms-tick PID-reuse race is already covered by the accept-time re-check.
3. Unverified claims. The doc is honest that the attacks were read from source and not reproduced. The code comment in KanataTCPClient.swift states the exposures as facts ("any local process … can … receive every KeyInput"), with a hedge only in the parenthetical. This is fine, but the lab-VM confirmation should be a tracked checklist item on Review the unauthenticated TCP endpoint for the session runtime #1354 so the "Proposed" status has a clear exit criterion.
4. Consistency. ADR-013's status line and the README and index rows are consistent with each other. The ADR-013 Context and Decision sections still say "Use TCP without authentication" with no inline pointer. That is acceptable because the note at the top covers it.

LGTM from a correctness standpoint.

[claude]

Review: docs/comment-only PR, no blocking findings.

The only code change is a doc comment in KanataTCPClient.swift. The rest is ADR text and index updates. Notes:

1. Unverified claims. ADR-046 says it was written from source alone. The comment in KanataTCPClient.swift hedges the same way, which is appropriate. The ADR should stay Proposed until the lab-VM check (second macOS user and sandboxed app both receive KeyInput) is done.
2. Phase 1 wording is inconsistent. The ADR says to bind once and keep the listener, which closes the check-then-use race. It also says the bridge's create_passthru_runtime takes a socket path. Make sure the fork API change and the removal of the bind probe land together. Otherwise the squatter case (attacker 4) stays open.
3. Owner-liveness check. The PID-reuse argument depends on the kqueue NOTE_EXIT watch. It is listed as an acceptance criterion, which is good. Add a test for the window between owner exit and the watch firing.
4. Release gate. The ADR notes that nothing enforces release-blocker issues in release-doctor.sh. Consider filing a follow-up so Review the unauthenticated TCP endpoint for the session runtime #1354 can't ship by accident.
5. Minor. The ADR-013 note is dated 2026-11-04, and the other ADR-046 dates match. No broken index links spotted.

No correctness, security or test-coverage issues in the diff itself.

[claude]

Review (docs/comment-only change; no code behavior touched)

No blocking findings. The KanataTCPClient.swift change is comment-only, and the ADR-013/README/index updates are consistent with each other and with the new ADR-046.

Minor, non-blocking notes on ADR-046:

• Phase 1 vs. Phase 4 ordering: the PID check alone is described as weaker than the signature check, but the release table still marks attacker 3 as "Partly mitigated". Consider stating in Review the unauthenticated TCP endpoint for the session runtime #1354 what the explicit release gate is, so phase 4 is not silently deferred.
• LOCAL_PEERPID caveat: the ADR already notes fd-passing/fork inheritance. It may be worth adding a Rust test for that case, or documenting it as a known gap, in the phase 1 acceptance criteria.
• Claims unverified: the ADR says it was written from source only. The proposed live-VM confirmation (second user and sandboxed app receiving KeyInput) should be tracked as a checklist item on Review the unauthenticated TCP endpoint for the session runtime #1354 before status moves to Accepted.
• The KanataTCPClient comment now says "from reading the source (not yet confirmed on a live install)". Remember to update it again once confirmed or once phase 5 lands.

🤖 Generated with Claude Code

[claude]

Review: docs-only change (ADR-046 + comment/status updates). No blocking issues. The Swift change is a comment edit only. Minor, non-blocking notes:

1. Hedged claims in a shipped comment. The KanataTCPClient.swift doc comment says "From reading the source (not yet confirmed on a live install)". That is honest, but the comment will go stale or misleading if the live-install check in ADR-046 finds otherwise. Add a TODO that points at Review the unauthenticated TCP endpoint for the session runtime #1354 so it gets revisited.
2. Phase 1 listener-failure handling. The ADR says to "return an error instead of panicking … by binding once and keeping that listener". It would help to state how the bridge reports that error to SessionRuntimeWorker. The worker should surface it as a failed launch with a clear state, not retry in a loop.
3. Phase 1 owner-exit race. The acceptance criterion relies on a kqueue NOTE_EXIT watch plus PID-version matching. Say what happens if the owner has already exited before the watch is registered. Registration should fail on ESRCH and trigger an immediate exit, and the required tests should cover that case.
4. sun_path fallback. Falling back to confstr(_CS_DARWIN_USER_TEMP_DIR) makes the socket location variable. Make sure verify-installed-app.sh and the CLI discover the path from the worker report only, never by assuming the primary directory.
5. Release-blocker enforcement. The ADR notes that nothing checks open release-blocker issues. Consider a follow-up issue so that gap doesn't get lost.

The docs are internally consistent. The ADR-013/README/index status updates match, and the ADR-046 links resolve.

[claude]

Review: docs/comment-only PR (ADR-046 + KanataTCPClient note). No code behavior changes. A few actionable points on the ADR text:

1. Phase 1 vs Phase 2 ordering/self-contradiction. Migration says "Phases 1 and 2 land together" in one fork update, but Phase 2 (bridge/worker API change to take a socket path + expected PID) is described as a separate KeyPath-side step. Clarify that the fork change and the bridge change are one atomic PR, otherwise a fast-forward-only keypath/bundled can't be bisected cleanly.
2. Phase 1 acceptance criterion relies on PID check that the ADR itself calls weak. The LOCAL_PEERTOKEN PID/pidversion check is documented as bypassable by fd-passing to a child or by any same-user process the app launches. Since attacker 3 is only "partly mitigated" at release, the "Decision" table and the Review the unauthenticated TCP endpoint for the session runtime #1354 release-blocker closure criteria should state explicitly whether attacker 3 residual risk is acceptable for release (right now it reads as an implicit yes).
3. Unverified claims are baked into the revised ADR-013 and the Swift doc comment as fact ("any local process, including other users' processes and sandboxed apps... can connect"). The ADR says these were not confirmed on a live install. The Swift comment hedges ("From reading the source"), but ADR-013's note and the README/index status rewording do not. Consider adding the same hedge, or run the lab-VM check before merging so docs don't need a second revision.
4. Dangling references. The ADR cites commit d9085a9 as being on a non-keypath/bundled branch, and e42ffea0 as the pinned master. These will rot; consider recording only the submodule path and noting "as of date" to keep the ADR durable.
5. Minor: KanataTCPClient.swift now has a TODO(#1354); fine, but make sure the TODO is tracked so it isn't stale when ADR-046 lands. Also the ADR-013 note date (2026-11-04) is later than the previous note (2026-10-31); consider keeping the original note for history rather than overwriting it.

No correctness, security, or test-coverage regressions in the code diff (comment-only). Nice, thorough threat model.

[claude]

Review (docs/ADR + comment-only Swift change)

No code-behavior changes, so no runtime risk. A few doc-accuracy notes:

1. ADR-046, Option C, attacker 3 / PID check: the doc says the PID + PID-version check blocks a direct connection from a same-user process, but also concedes a connection the app hands to a child passes. Consider stating plainly in the Decision table that phases 1–3 give only weak protection against attacker 3 (a same-user process can still trigger the app via keypath:// URLs, which the ADR already notes). The wording is mostly there; just make sure the release gate decision is explicit for reviewers.
2. Phase 1 acceptance test: the NOTE_EXIT watch registration race (ESRCH) is covered, but there is no test listed for the sun_path 104-byte fallback to confstr(_CS_DARWIN_USER_TEMP_DIR). Suggest adding one to Phase 2/3.
3. ADR-013 / README / index status wording is consistent across the three files.
4. KanataTCPClient.swift comment: the TODO(#1354) is fine; the claims are hedged as source-read, not live-verified, which matches the ADR. The ADR cites fork commit d9085a9 on a non-bundled branch, so the facts could drift once that merges; re-verify when the submodule pointer moves.

No blocking findings.

[claude]

Review: one blocking finding, one minor.

1. Blocking: CGSPrivate.swift and WindowManagerTests.swift undo the earlier dlsym hardening, and the PR description doesn't mention it. The PR is meant to be docs plus a comment fix (ADR-046, ADR-013, and the KanataTCPClient note). It also deletes CGSFunctions and brings back the @_silgen_name declarations. It removes the three resolve/resetCache tests too.

   • Launch risk: @_silgen_name makes the linker emit strong undefined imports. The removed comment says dyld then refuses to launch the app if a future macOS drops any one of these symbols. The runtime isAvailable check can't help, because the process never starts.
   • Memory risk: the old code used Unmanaged<CFArray>? and takeRetainedValue() for CGSCopyManagedDisplaySpaces and CGSCopySpacesForWindows, because they follow the Copy rule and return +1. The restored declarations return a bare CFArray. Swift treats that as +0, so the arrays leak on every refreshSpaces() and moveWindow() call. A nil return would also crash instead of hitting the guard.
   • Thread safety: _isAvailable is back to unsynchronized nonisolated(unsafe) state.
   • Likely cause: this looks like a stale base or an accidental revert. Please rebase on current master and drop these two files from the diff. If the revert is intentional, say why, and restore the resolve tests.

2. Minor, docs/adr/adr-046-session-engine-control-channel.md: the ADR cites source at d9085a9 on a fork branch that isn't on keypath/bundled, and it says the claims are untested on a live install. Merging it as "Proposed" is fine, but the KanataTCPClient note now states the exposure as fact, hedged with "from reading the source". The TODO(Review the unauthenticated TCP endpoint for the session runtime #1354) there covers the follow-up.

The ADR content and the ADR-013 cross-links look consistent.

[malpern]

The CGS finding in the previous review is a stale-base artifact. master gained #1359 (dlsym for private CGS symbols) after this branch last merged it, so a two-dot diff showed that change reversed. This PR never touched CGSPrivate.swift or WindowManagerTests.swift. master is now merged in (8e5235cf). The PR diff is the five ADR/comment files only: KanataTCPClient.swift (comment), docs/adr/README.md, docs/adr/index.md, adr-013, adr-046.

[claude]

Review: docs/ADR plus a comment-only Swift change. No blocking issues.

Minor points:

1. Cross-doc consistency. ADR-046 says the current d9085a9 fork commit is on keypath/explicit-source-keys-20261017, not yet on keypath/bundled. AGENTS.md says the submodule tracks keypath/bundled. Before accepting the ADR, confirm the claims still hold for whatever commit ships. Otherwise the "facts read from source" could drift.
2. ADR-013 wording. The edit to the "Same security model as other local IPC (Unix sockets, etc.)" line is inline and correct. The Date: 2024 header is unchanged, which is fine.
3. Phase 1 / Phase 2 coupling. The ADR says phases 1 and 2 land together in one KeyPath PR, and it also calls the fork branch "a new branch merged into keypath/bundled by fast-forward". Both are consistent with the fork-safety rules. A short checklist item to verify fast-forward (git log --oneline <source>..<target>) in the implementation PR would help.
4. KanataTCPClient.swift TODO. TODO(#1354) is tracked, which is good. Nothing here changes behavior, so no tests are needed.
5. Process gap noted in the ADR. Nothing enforces "no open release-blocker issues" at release time (release-doctor.sh). Consider a follow-up issue so Review the unauthenticated TCP endpoint for the session runtime #1354 can't be shipped over by accident.

Nothing in the diff affects runtime behavior. The ADR's threat model and phased plan look internally coherent.

[claude]

Review: docs-only ADR plus a comment change. No blocking findings.

The only code change is the doc comment in KanataTCPClient.swift. It has no behavior impact.

Minor points:

1. Dates. ADR-046 and the ADR-013 note use 2026-11-04. The ADR-013 note was added under that date, but the earlier one is 2026-10-31. Check that these match the actual authoring dates.
2. Pinned commit. ADR-046 relies on fork commit d9085a9, which the ADR says is not yet on keypath/bundled. The "re-check before accepting" step already covers this. When the ADR moves to Accepted, record the final commit hash.
3. Phase 1 liveness. The kqueue NOTE_EXIT watch and the PID-version comparison are described only as acceptance criteria. Add a note that LOCAL_PEERTOKEN may not be available on every supported macOS version. Phase 1 depends on it, so confirm this during implementation, as is already done for NWEndpoint.unix.
4. TODO(Review the unauthenticated TCP endpoint for the session runtime #1354). The TODO in the Swift comment is fine. It points at a tracked issue.

The threat model is thorough. It also says honestly that the claims come from source reading and need confirming in a lab VM.

🤖 Generated with Claude Code

[claude]

Review of #1358 (ADR-046 + KanataTCPClient comment update)

No blocking findings. The Swift change is comment-only, and the doc and index updates are consistent with each other (README, index.md and ADR-013 status all match).

Minor points for the ADR:

1. Phase 1 acceptance criterion vs. the 5 ms tick. The ADR says the PID check is meaningless without the kqueue NOTE_EXIT watch plus the PID-version comparison. Both belong in the same PR as the Unix listener, which the doc already says. Make sure the Phase 2 KeyPath PR cannot merge without the matching Rust tests. The ADR calls them required but nothing enforces it.
2. LOCAL_PEERTOKEN availability. The ADR relies on it for PID version and for audit-token-based signature checks. Confirm in the lab VM spike that it works on AF_UNIX stream sockets on the minimum supported macOS, and not only via getpeereid. This is the core of the attacker-3 mitigation.
3. Release-blocker enforcement. The Consequences section says nothing enforces "no open release-blocker issues". Consider filing a follow-up so Review the unauthenticated TCP endpoint for the session runtime #1354 can't be missed at ship time.
4. Claim status. The ADR and the code comment are explicit that the cross-user and sandbox claims are source-derived and unverified. The TODO(#1354) in KanataTCPClient.swift is a good marker. Don't flip ADR-046 to Accepted before the lab-VM check.

Docs-only change, so no test coverage is needed.

[claude]

Review: docs-only PR (ADR-046 plus a comment edit in KanataTCPClient.swift). No blocking findings.

Minor, non-blocking notes:

1. Rust expect panic vs. catch_unwind. The ADR says the TcpListener::bind(...).expect panic "aborts the worker" inside an extern "C" function. That depends on the Rust edition and panic strategy, since unwinding out of extern "C" aborts on current Rust. Worth stating the assumption, because phase 1's "return an error instead of panicking" fix relies on it.
2. Phase 1 test plan. It needs a way to fake the peer's PID version without a real second process. The ADR says "simulated in the check", so make the peer-token reader injectable so that test is deterministic.
3. Release-blocker gate. The ADR notes nothing enforces open release-blocker issues. Consider a follow-up issue to add a release-doctor.sh --ship check, so Review the unauthenticated TCP endpoint for the session runtime #1354 can't be bypassed.
4. KanataTCPClient.swift comment. The TODO references Review the unauthenticated TCP endpoint for the session runtime #1354, which is fine. The comment states the source-review claims as fact ("any local process, including other users' processes and sandboxed apps … can"), but the ADR says these are not yet confirmed on a live install. The "(not yet confirmed…)" hedge is there, so this is fine as is. Remove it once the lab-VM check completes.
5. ADR-013 edit. The two inline "(Wrong: …)" annotations are clear. Note the date header on the second note (2026-11-04) is later than the ADR-046 date only by convention, so that is consistent.

No code behavior changes, so no test coverage is needed for this PR.

[claude]

Review: docs-only ADR plus a comment change in KanataTCPClient.swift. No code behavior changes, and I found no blocking issues.

Minor notes, all non-blocking:

1. Date inconsistency. The ADR-013 note says "Note (2026-10-31)" and then "Note (2026-11-04)", and ADR-046 is dated 2026-11-04. Please check these against the actual dates.
2. PID-based peer check (Option C, phase 1). The ADR already admits that a connection handed to a forked or exec'd child still passes, and that phase 4 closes the gap. It might be worth saying in the Decision section that LOCAL_PEERTOKEN is unverified on macOS 15. The ADR does list this as a pre-acceptance check, so this only asks that the same caveat appear next to the release-gate wording.
3. Source-only claims. The ADR says its claims come from reading the source and were not confirmed on a live install. The KanataTCPClient comment also says "from reading the source". That is fair and honestly worded. The TODO(#1354) ties the comment to the issue, so it will not go stale unnoticed.
4. Test coverage. The ADR says CI runs no Rust tests today and plans a cargo test step with the phase 1 and 2 PR. That is the right gate. Make sure the required acceptance tests (kill the owner, PID-version mismatch, ESRCH on registration) are listed as merge-blocking when that PR lands.

Otherwise the cross-links, the README and index tables, and the ADR-013 status updates are consistent.

🤖 Generated with Claude Code

[claude]

Review: docs/comment-only PR (ADR-046 + KanataTCPClient note). No code behavior changes.

No blocking findings. A few minor, actionable notes:

1. Status inconsistency. ADR-013's header says "proposed to be superseded by ADR-046", while its body notes assert present-tense facts ("Wrong: loopback TCP has no peer-uid check…"). Fine, but the KanataTCPClient.swift comment states exposure claims ("any local process, including other users' processes and sandboxed apps…") as fact while hedging "not yet confirmed on a live install". Consider keeping the hedge in the ADR-013 inline notes too, so the claims are consistently marked unverified until the lab-VM check in Review the unauthenticated TCP endpoint for the session runtime #1354 completes.
2. ADR-046 Phase 1 depends on unverified OS behavior (LOCAL_PEERTOKEN on AF_UNIX stream sockets, audit_token_to_pidversion, NWEndpoint.unix on macOS 15). The ADR already calls for confirming these; suggest making that an explicit checklist item on Review the unauthenticated TCP endpoint for the session runtime #1354 so Phase 1 can't start before it's answered.
3. Date consistency. ADR-013 note is dated 2026-11-04 and ADR-046 says "As of 2026-11-04"; fine, but the pinned commit hashes (d9085a9, e42ffea0, 54cdba9) will go stale. The ADR does say to re-check before acceptance, which is sufficient.
4. TODO(Review the unauthenticated TCP endpoint for the session runtime #1354) in source is appropriately tracked; no action needed.

Docs index/README entries are consistent with each other. No tests are affected.

[claude]

Review: docs/comment-only PR (ADR-046 + KanataTCPClient note). No code behavior changes.

No blocking findings. A few small, actionable notes:

1. ADR-013 / README / index status wording: README and index now say "Under review; proposed to be superseded by ADR-046", while ADR-013's own header uses the same text. Consistent, good. Once ADR-046 is accepted, remember to flip all three (plus ADR-046's own "Supersedes (when accepted)") together.
2. ADR-046 internal consistency: The attacker-3 mitigation at phases 1–3 relies on LOCAL_PEERTOKEN working on AF_UNIX sockets (macOS 15). The ADR correctly flags this as unverified, but "Decision (proposed)" and the release gate ("phases 1 to 3") are written as if it holds. If the lab check fails, the fallback for binding a connection to the owner is only described vaguely ("another way"). Consider stating the fallback now (e.g. LOCAL_PEERPID plus the NOTE_EXIT watch, with the PID-reuse window documented as accepted).
3. Date/claim hygiene: The ADR and the swift comment state source-derived claims "not yet confirmed on a live install". The TODO(#1354) in KanataTCPClient.swift is a good tracker; make sure the issue actually lists the lab-VM verification as a checklist item so the TODO doesn't go stale.
4. Docs build: the ADR index uses /adr/adr-046-session-engine-control-channel (extensionless) like the other rows, so links look fine. The long unwrapped lines in the ADR are harmless.

Nothing here affects runtime, tests, or the Swift build (the only Swift change is a doc comment).

[claude]

Review: docs-only change plus a comment edit in KanataTCPClient.swift. No code behavior changes, so nothing here can break at runtime.

Actionable notes, all minor:

1. ADR-046 internal consistency. The Decision says phases 1–3 gate the release, and phase 1 requires a cargo test CI step. The ADR also says CI runs no Rust tests today. Track that CI work explicitly (an issue or checklist item in Review the unauthenticated TCP endpoint for the session runtime #1354), or it can slip once the ADR is accepted.
2. LOCAL_PEERTOKEN on AF_UNIX is an unverified load-bearing assumption. The attacker-3 mitigation and the PID-version race fix both depend on it. The ADR flags this for the lab VM check. Make that check an explicit blocker for moving the status from Proposed to Accepted, not only a pre-acceptance suggestion.
3. Status wording. ADR-013 now reads "Under review; proposed to be superseded". The KanataTCPClient comment and the ADR text both say the claims come from a source read and were not confirmed live. That is appropriately hedged. The TODO(#1354) in the code is fine, but make sure the issue stays open to carry it.
4. Minor doc nit. adr-013 inline "(Wrong: …)" annotations are clear. The two notes at the top use different dates (2026-10-31 and 2026-11-04), which is fine, but check that the dates match the repo's date convention.

No correctness, security or performance issues in the diff itself. LGTM from a review standpoint.

[claude]

Review: no actionable findings. This PR changes docs and one code comment, with no behavior change. The ADR index, README and ADR-013 status lines agree with each other. The updated KanataTCPClient security note matches ADR-046 and points at #1354.

Two non-blocking notes:

• ADR-046 keeps its main claims unconfirmed on purpose (second user and sandboxed app receiving KeyInput, LOCAL_PEERTOKEN on macOS 15). It stays Proposed until the lab-VM check runs, so Review the unauthenticated TCP endpoint for the session runtime #1354 should keep its release-blocker label until then.
• The TODO(#1354) in the Swift comment is fine. Remove it when ADR-046 lands, so the stale-note fix is not lost.

🤖 Generated with Claude Code