fix(ipc): authenticate process calls and survive a dead repair client

[Raghav-Bansal-15]

IPC callers could claim identities without proving them, and a dead adviser could interrupt later recovery. The incompatible authenticated IPC jar also retained its old artifact version.

Pin endpoint certificates, issue revocable per-instance credentials, enforce service authority and scrub subprocess credentials. Publish the contract as IPC 1.1.0 and preserve the separate transport marker. Contain non-cancellation adviser failures and test the real request boundary with a retained dead client. Refuse handler dispatch when revocation wins between identity lookup and listener registration; a deterministic regression fails before this fix.

Validation at 4f209d77: 62 IPC tests, 4 desktop recovery tests, applicable quality checks and contract-jar assembly pass locally. Runtime #21 is paired with this head; its 262 local tests and fatJar build pass. Coordinated distribution, native runtime validation and the minimum supported application release remain release gates; nothing is released.

Original and reviewer commits are preserved. Upstream workflow approval and required checks are separate gates; no merge or release.

Native-image follow-up: protobuf descriptors now initialize at runtime to avoid the macOS build-time initialization cycle. Current head 1ccec3f6; local IPC tests/JVM smoke/quality and paired runtime build pass. Current-head hosted checks passed, including the three-OS native build and security smoke tests.

Current-head hosted evidence: 🔨 Build & Test, Authenticated IPC boundaries.

[kshivang]

@claude Please review current head 96aec67 against dev, focusing on authenticated IPC transport, credential lifecycle, process/terminal ownership, host/runtime compatibility and cross-platform CI. Inspect companion runtime PR #21 read-only as needed; do not mutate it. This is the first review request for this head.

[claude]

Claude finished @kshivang's task in 10m 26s —— View job

---

Review: authenticated IPC at 96aec67

• Gather the diff against dev
• Authenticated IPC transport (TLS pinning, address resolution)
• Credential lifecycle (mint, replace, revoke, environment stripping)
• Process / terminal ownership enforcement
• Host / runtime compatibility contract
• Cross-platform CI workflow
• Companion runtime PR chore(build): Gradle 10 readiness — typed source-set accessors, project-notation fix, grpc-kotlin codegen alignment #21 — could not inspect, see caveats
• Post review

Caveats on what I could verify. This is a shallow checkout with no Gradle cache and no build permission, so I did not run test, ktlintCheck, detekt, nativeCompile or the smoke binary — everything below is from reading the diff (b39c756..HEAD, the three PR commits). gh access to risa-labs-inc/boss-microkernel-runtime is not available from this job, so PR #21 is unread; I've marked the places where the contract's other half is what decides correctness.

The shape of this is right, and two things in particular are the correct instincts: migrating the existing test suites onto IpcTestServer so they exercise the authenticated transport rather than bypassing it, and SecureSpawnTest + TerminalOwnershipProcess.java proving the environment property by spawning a real JVM that inspects its own environment and fails loudly. Reordering putAll(config.environment) before the BOSS_* puts in ProcessSpawner also closes a genuine spoofing hole that the old comment claimed was closed and wasn't.

---

Blocking

1. A dead orchestrator permanently kills the kernel's crash-recovery monitor.

KernelBootstrap.kt:615-618 now reads the channel off the registry:

val client = processRegistry?.getProcess(ORCHESTRATOR_PROCESS_ID)?.ipcClient ?: return null
return OrchestratorServiceGrpcKt.OrchestratorServiceCoroutineStub(client.channel)

Three facts combine badly:

• ProcessRegistry.register documents that it deliberately retains dead handles ("Replacing a dead handle is normal - that is what a respawn does"), so getProcess returns a ManagedProcess for an orchestrator that has already exited.
• SpawnIpcSecurity.revoke() runs on process.onExit() and calls client.shutdown(0), which sets closed = true.
• BossIpcClient.channel is now check(!closed) { "IPC client has been closed" }.

So client.channel throws where the old cached-channel path returned a dead-but-usable channel. And orchestratorStub() is called at :578, outside the try at :594 that exists precisely to turn "cannot reach the orchestrator" into null. handleFailure is launched at :497 as launch { … } from inside the failures.collect coroutine — a regular child, not a supervisor child — so the exception cancels the collecting coroutine. The SupervisorJob at :355 stops it spreading further but does not restart the collector.

Failure scenario: the orchestrator crashes; its registry entry survives; the next child process to fail makes requestRepairAdvice throw; processMonitor.failures.collect is cancelled; no further process failure is handled for the rest of the session, including respawns. That is strictly worse than the pre-PR behaviour, where the same situation degraded to one logger.warn per failure.

Either move the orchestratorStub() call inside the existing try, or gate it on the client still being open (BossIpcClient currently exposes no such accessor — isConnected returns false for a never-built channel too, so it can't stand in). Fix this →

2. IpcVersion.CURRENT is still 1.0.0, so four existing compatibility surfaces lie about an old runtime.

IpcTransport.requireCompatibleRuntime is enforced in exactly one place — OutOfProcessPluginSpawnerImpl.validateRuntime, at spawn. But minIpcVersion is already the repo's mechanism for "this runtime cannot talk to this host", and it is read in four other places that all still answer "compatible":

Surface	What the user sees
IpcCompatibility.isInstallable / StoreHomeCatalogProvider:140	store badge says installable
PluginStoreSetup.ipcIncompatibilityReason (:1462, :2147)	adopt/install gate passes
PluginUpdateManager:306	no incompatibility notice
StoreRepairArtifact:42	repair promotes the artifact

So a user on this host can browse, download and adopt a pre-contract runtime with no signal at all, and meets the failure at first spawn as logger.error("Failed to spawn out-of-process plugin: manifest={}") plus a Result.failure whose message is now just the marker text — the old code's message carried the host IPC version and the runtime path, which is what makes it actionable.

Bumping CURRENT to 2.0.0 lights all four up with the existing "Update the runtime JAR." wording and rejects before download, leaving the marker as belt and braces for a runtime that lies about its version. The KDoc's bump policy is phrased around src/main/proto/ changes, which this PR doesn't touch — but the transport is a wire-level breaking change by any other reading, and BossIpcServer, BossIpcClient and ChildProcessBootstrap.connectToService are source- and binary-breaking changes to a jar AGENTS.md says the runtime repo consumes as a published upstream (modules/boss-ipc/build.gradle.kts:37 still says version = "1.0.0" too). If the deliberate decision is not to bump, that reasoning belongs in IpcVersion's KDoc, because it currently reads as a rule this PR broke. Fix this →

3. The new workflow's GraalVM step is misconfigured, and its path filter excludes the host half of this PR.

.github/workflows/authenticated-ipc.yml:44-48:

- uses: graalvm/setup-graalvm@v1
  with:
    version: '25.0'
    distribution: graalvm

version is the legacy input for GraalVM releases up to 22.3 (and dev/mandrel); with distribution: graalvm the action expects java-version: '25'. As written this step is likely to fail resolution on all three runners, which means the "Build and exercise native authenticated IPC" job — the one that actually runs the smoke binary — never proves anything. (I couldn't execute it to confirm; please run workflow_dispatch once before merge.)

Separately, pull_request.paths covers only the four modules/** directories. KernelBootstrap.kt, OutOfProcessPluginSpawnerImpl.kt and PluginUIServiceBridgeTest.kt are changed by this PR and are the host side of the contract, so the workflow will not run on a future PR that touches only them. And push: branches: ['fix/security-authenticated-ipc'] is branch-specific scaffolding that should come out before merge.

Also note toolchainDetection.set(false) in modules/boss-ipc/build.gradle.kts:11 is unlike the nine sibling modules that use graalvmNative — it makes nativeCompile take native-image from whatever JAVA_HOME is, which is the whole reason the GraalVM step's correctness matters. The Java 17 toolchain at :39-43 does keep compilation on 17, so that part is fine. (I can't edit .github/workflows/ — GitHub App permissions exclude it — so this one needs a human commit.)

4. Ten unused imports will fail ktlintCheck.

no-unused-imports is not among the rules .editorconfig disables:

• IpcRoundTripTest.kt:6,7,16 — ManagedChannelBuilder, ServerBuilder, ServerSocket
• StateServiceTest.kt:6,7,15 — same three
• EventBusServiceTest.kt:6,7,18 — same three
• LateServiceRegistrationTest.kt:10 — ManagedChannelBuilder

All four files dropped their hand-rolled server/channel setup for IpcTestServer and kept the imports. LateServiceRegistrationTest is the unambiguous case (no wildcard import in the file). The PR body says ktlint passed, so either the run predates the IpcTestServer migration or something is masking the rule — worth re-running :boss-ipc:ktlintCheck. ./gradlew ktlintFormat fixes it.

---

Medium

5. The runtime contract marker is an exact byte match, and a single string. IpcTransport.kt:24 compares readNBytes(64) against "pinned-tls-v1;subprocess-env-v1\n" verbatim. A runtime jar whose resource picked up CRLF, lost the trailing newline, or gained a comment line fails with "uses an unsupported IPC transport" — indistinguishable from a genuinely old runtime, and the diagnosis is invisible because the value is never logged. More structurally: because it's one exact string, no single runtime build can satisfy two host generations, so every future contract bump is a flag day across both repos. value.trim() against a Set of accepted tokens costs nothing now and buys the overlap window later.

6. watchState tears down a stream because someone else wrote the key. StateServiceImpl.kt (watchState) authorizes the initial read only if the key already exists. A watcher that subscribes to a key not yet in the store passes, then authorizeRead inside .collect throws PERMISSION_DENIED the first time a different instance writes it — killing the RPC rather than filtering the event. IpcAuthorizationTest covers the already-exists case (refused(PERMISSION_DENIED) { beta.watchState(key).toList() }), not this one. The emission-time check should filter, not require, and keep a require only for the initial snapshot.

7. getState is a key-existence oracle. A missing key returns an empty StateValue with version = 0; an existing foreign key returns PERMISSION_DENIED. Any authenticated process can therefore enumerate which keys exist across instances. listStateKeys filters correctly, so this is the one hole. Returning the empty default for an unreadable key would close it (at the cost of a less honest error).

8. principalFor takes a global monitor on every streamed message. ProcessTokenRegistry.principalFor is @Synchronized, and ProcessIdentityInterceptor's sendMessage override calls it per response message. Terminal output streaming is the highest-frequency path in the tree and it now serialises through one lock shared with every other stream and with issue/revoke. Keep the lock for mutation and make the read path a ConcurrentHashMap; IssuedCredential already carries its own AtomicBoolean, so the map value is safe to read unlocked.

9. Cross-thread ServerCall.close() races the handler. The onRevoked listener calls guardedCall.close(...) from the revoking thread. finished (AtomicBoolean) correctly serialises close-against-close, but not sendMessage against close: a handler thread can pass !finished.get(), the revoking thread can then CAS and super.close(), and the handler's super.sendMessage() lands on a closed ServerCallImpl and throws IllegalStateException. Practically that surfaces as a failed coroutine on a call that is already closed, so it's survivable — but it deserves a comment saying so, because the next reader will assume finished made it safe.

10. Two load-bearing comments were deleted, both the kind AGENTS.md exists to keep.

• ProcessTokenRegistry's threat-model paragraph is gone: "This authenticates possession of a child credential, not an OS sandbox boundary. The environment can be inherited by descendants or inspected by same-user processes." It was deleted in the same change that put a TLS private key (SERVER_PRIVATE_KEY) and a HOST-authority token (HOST_TOKEN) into that same environment. The limitation got strictly more consequential and its only written record disappeared. It belongs on IpcEnvironment, next to removeCredentials, which is now the mitigation.
• ProcessSpawner's @JvmOverloads rationale is gone ("load-bearing, not style … exactly the failure KernelReflectionContractTest exists to catch"). The annotation survives at :25 and the new 5-arg shape still generates the 3-arg constructor, so nothing is broken today — but the next person to touch that parameter list has lost the reason not to.

11. Server-auth-only TLS, no mTLS, and Windows has no filesystem equivalent of secureSocketFile. Authorization is the bearer token in boss-process-token; the certificate pin proves which endpoint you reached, not who you are. On UDS that composes with IpcAddressResolver.secureSocketFile; on Windows the transport is TCP loopback, where the token is the only control. That is a defensible design, and the pinning plus per-incarnation revocation is a real improvement — it just needs stating now that (10) removed the paragraph that used to state the analogous limit.

---

Low / nits

12. SpawnIpcSecurity.create mints the host token via ProcessTokenRegistry().issue("host", HOST) on a throwaway registry used purely as a CSPRNG, then discards the object. A newToken() helper would say what's happening. Relatedly, the onFailure { security?.revoke() } path is leak-free only because registry.issue happens to be the last statement in create that can throw — worth a comment or an explicit try, since reordering it silently leaks a live credential for a process that never started.

13. ChildProcessBootstrap.connectToService is if (serviceName !in serviceAddresses) return null followed by an unconditional error(...), so a function declared BossIpcClient? can never return non-null. Make it throw unconditionally, or delete it and the now-dead serviceAddresses plumbing on ChildProcessConnection.

14. ProcessTokenRegistry.issue constructs a fresh SecureRandom() per call; the previous code held one instance.

15. KernelServiceImpl.registerProcess moved the registeredProcesses / lastHeartbeats writes to after onProcessRegistered, so the callback now observes a kernel that doesn't yet list the process registering. The trailing second requireOwnProcess explains itself; the reorder doesn't.

16. UnknownRuntime and a manifest read failure are now both fatal where they used to warn-and-continue, and error(compatibility.reason) drops the host IPC=…, runtime=… suffix the old message carried. The strictness is right; the message regression is not.

17. modules/boss-process-manager/build.gradle.kts:39 creates Files.createTempDirectory("bs") per test run and never deletes it.

18. TerminalOwnershipProcess.main ignores args, so request("wait"), request("echo") and request("sentinel") are identical. More importantly, when its credential assertion fires the sentinel is never written, so the test fails as a 15-second withTimeout rather than naming the security property it was proving — asserting on the child's exit code or streamed stderr would make that failure legible.

19. IpcAddressResolver accepts uri.port in 0..65535. Port 0 is correct for a server bind (IpcTestServer relies on it) and meaningless for a client dial; the two paths could be split.

20. IpcTlsIdentity certificates are valid 366 days and checkValidity() runs only at context construction. Fine for any realistic host lifetime, but a re-dial after a year fails with a TLS error rather than anything that names expiry.

21. OrchestratorAuthorizationTest depends on ProcessAuthority.entries declaration order (HOST last) for assertEquals(0, restarts.get()) to hold. Reordering the enum fails loudly rather than silently, so this is cosmetic — but iterating non-HOST authorities explicitly would say what it means.

---

On the companion runtime, unverified

I couldn't read PR #21, so these are the three things I'd check on that side before either merges:

• The marker resource must be byte-identical, pinned-tls-v1;subprocess-env-v1 plus exactly one \n, at META-INF/boss-runtime/security-contract, and must survive whatever shading the runtime jar goes through. See (5).
• EventBusService.publish/publishBatch now overwrite EventEnvelope.sourceProcess with the credentialed process id, and StateService.setState ignores StateUpdate.sourceProcess entirely (writing caller.processId instead). In-repo this is safe — IpcEventBridgeImpl uses publishLocal, which preserves the field — but any runtime-side or analytics-side publisher that set a logical source now gets pluginProcessId(windowId, pluginId).
• connectToService no longer connects. Any runtime peer-to-peer path must already be routed through the host, and the failure mode is a thrown IllegalStateException from a nullable-returning function. See (13).

Nothing in this PR was mutated, and PR #21 was not touched.
· branch fix/security-authenticated-ipc

[kshivang]

@Raghav-Bansal-15 Your per-endpoint TLS pinning, per-incarnation credentials, process/terminal ownership and real spawned-JVM environment checks address important IPC gaps. I reviewed all three authored commits and the completed Claude review. This needs an author handoff before it is suitable for dev.

Substantive blocker verified in source: KernelBootstrap.requestRepairAdvice obtains orchestratorStub() before its try/catch. The stub now reads client.channel, which throws after SpawnIpcSecurity.revoke() closes that client on child exit. The registry deliberately retains dead handles. A subsequent process failure can therefore throw out of the failures collector and disable recovery for the session. Move channel/stub acquisition into the recover-without-advice error boundary and add a regression using a retained dead orchestrator handle: the failure returns null advice, and a later failure is still handled. Do not swallow coroutine cancellation while doing this.

Bounded reviewer repair: corrected the authenticated-IPC workflow to java-version: '25' (verified against setup-graalvm's actual action inputs), included host kernel/spawner/test paths in its trigger, and removed ten confirmed unused imports left by the IpcTestServer migration. These are reviewer changes, not your original contribution. They do not fix the recovery blocker and have not completed validation.

Compatibility/rollout remains coordinated work: the marker is checked at runtime spawn while the existing protobuf/minIpcVersion surfaces still describe a different contract. Document and validate how old runtime artifacts are refused or excluded before adoption/distribution and how a matching host/runtime pair is delivered. I did not blindly bump every IPC major to 2: the code explicitly separates transport from protobuf compatibility, and a blanket bump affects all existing plugin declarations. Companion runtime PR #21 is read-only/outside this assignment, not permission to publish it. Its main/source channel wiring and PR metadata were inspected here, but its packaged behavior and remote checks remain unverified.

Additional valid review follow-ups to resolve or explicitly justify with tests: unreadable state keys reveal existence via differing responses; subscribing before a foreign key appears terminates the stream (choose/document the intended refusal semantics); restore the bearer-token/same-user threat-model documentation. I did not apply speculative lock-free registry changes, relax the exact contract marker, or remove peer-discovery APIs merely for style: those change security/concurrency/compatibility policy and require their own evidence. The companion PR includes newline controls, so CRLF rejection is not itself proof of a shipped defect.

Validation blockers are separate: both original-head workflows are action_required (34597521662 and 34597521701), requiring maintainer fork approval, not an observed test failure. The local shared-lock request for IPC/JVM smoke/process/terminal/orchestrator/host bridge tests plus ktlintCheck/detekt remains queued; no independent pass is claimed. Native Linux/macOS/Windows and matching packaged-runtime validation must pass on the repaired head before undrafting. Keep original authorship and the reviewer changes separate.

Per the user's bounded-review policy I am converting this open PR to draft after this handoff. No merge, admin bypass, live app operation or runtime release. The useful security work is preserved; the outstanding recovery defect and validation/rollout requirements are explicit rather than being treated as a green review.

[kshivang]

Validation update after the handoff: reviewer head 45edecf passed the shared-lock run. JUnit reports contain 200 tests total (IPC 60, process-manager 38, terminal 3, orchestrator 77, host UI bridge 22), zero failures/errors/skips. nativeSmokeJvm and root ktlintCheck/detekt also passed. The handoff remains in force: these tests do not cover the verified dead-orchestrator channel acquisition failure, and native/packaged-runtime CI and rollout validation remain outstanding. This updates the earlier queued-local-validation observation, not the draft disposition or original contributor score.

[kshivang]

@Raghav-Bansal-15 I verified the contributor follow-up against the prior handoff: newer ProjectData/SplitView/RunConfig test fixtures use pinned TLS and preserve anonymous/revoked refusal and provider-not-called assertions. Guarded repair-client acquisition and cancellation behavior remain present. Companion runtime #21 now records paired host/API 1.0.93 validation; coordinated distribution/minimum-host decisions remain explicit release gates, not permission to publish anything.

Merged rebased dev 77c4a4071 in reviewer head a9f58658e, retaining all author and previous reviewer commits. No new source repair in this pass. Integrated IPC/process/terminal/orchestrator and host bridge/repair tests plus applicable ktlint/detekt are queued under the shared lock; no fresh pass is claimed yet.

Cross-group context: unassigned draft #669 contains unique generation/collision/rollback safeguards in KernelServiceImpl. It is not a complete duplicate of this TLS/authority/ownership change; its implementation remains untouched and must be reconciled by its owner. #581 depends on this transport and also stays separately owned.

@claude Please review this current head once against dev, focusing on prior recovery and fixture-integration findings, revocation, and compatibility boundaries. Do not edit code. Exact-head upstream CI and substantive review disposition remain merge gates. No runtime release or app launch performed.

[claude]

Claude finished @kshivang's task in 9m 37s —— View job

---

Review: authenticated IPC at a9f5865 vs dev

• Gather the diff against dev
• Recovery finding: dead orchestrator client / repair advice
• Fixture integration: ProjectData / SplitView / RunConfig bridge tests
• Revocation semantics
• Compatibility boundaries
• Post review

What I could verify. Shallow checkout, no Gradle cache, no build permission, so nothing below was executed. origin/dev in this checkout is b461f18 with no common ancestor available, so I diffed against 77c4a40 (the dev commit the merge at a9f5865 brought in) - that isolates the PR's own changes. Runtime #21 is unreadable from this job, so the contract's other half is unverified. I did not edit code.

---

Previously blocking, now resolved

Each of these I re-checked in source rather than taking the description on trust.

Finding	Disposition
Dead orchestrator kills the failure collector	Fixed. orchestratorStub() moved inside repairAdviceOrNull (KernelBootstrap.kt:608-611), which rethrows CancellationException before any other catch, so caller cancellation survives.
Ten unused imports	Fixed. All four migrated IPC tests are clean.
Workflow GraalVM step	Fixed to java-version: '25'; paths now covers kernel/**, OutOfProcessPluginSpawnerImpl.kt and desktopTest/.../kernel/**.
Deleted threat-model paragraph	Restored, on IpcEnvironment next to removeCredentials - the right home, since that is now the mitigation.
Dev fixture integration	Done. ProjectData / SplitView / RunConfig build NettyServerBuilder.sslContext(tls.serverContext()) with overrideAuthority(IpcTlsIdentity.AUTHORITY). Importantly the assertion split is correct, not a blanket sed: anonymous and revoked callers assert UNAUTHENTICATED, while RunConfigServiceBridgeTest keeps assertFailsWithPermissionDenied for the authenticated-but-unauthorized execute cases (:230, :251, :254), and the provider-not-called and revoked-stream assertions are intact.
State key-existence semantics	Documented explicitly in docs/authenticated-ipc-rollout.md:23-27, including the watch-an-absent-key termination. That is the "choose and document" disposition that was asked for.

---

Remaining: one compatibility boundary that is mechanically checkable

1. :boss-ipc still declares version = "1.0.0", and that string names the released artifact.

This is a sharper version of the earlier IpcVersion.CURRENT finding, and it does not depend on the protobuf-versus-transport argument at all.

• gradle/upstream-artifacts.gradle.kts:32 registers verifyIpcVersionConsistency, which fails the build unless IpcVersion.CURRENT == :boss-ipc.version.
• assembleUpstreamJars copies boss-ipc-<that version>.jar into build/upstream-artifacts/, and release.yml:1591 attaches it to every BossConsole release with the comment "File names are <module>-<ipcVersion>.jar … so consumers can pin by IPC version."

This PR breaks that jar's public API three ways: BossIpcServer's constructor gains two required parameters, BossIpcClient's gains a required IpcClientCredentials, and ChildProcessConnection.connectToService changes from "returns null" to "throws IllegalStateException". So the next release publishes a different, source-incompatible boss-ipc-1.0.0.jar under a filename the runtime repo is told to pin by. A consumer holding an earlier download of that exact name and a consumer fetching the new one get different APIs with no way to tell them apart.

The earlier objection to bumping was that it "affects all existing plugin declarations". That objection applies to a major bump and not to a minor one. Read IpcVersion.isCompatible: within one major, a runtime is rejected only when rtMM > hostMM. Moving the host to 1.1.0 therefore

• rejects nothing that 1.0.0 accepted (every existing minIpcVersion: 1.0.x declaration still passes),
• makes an old host refuse a runtime that declares 1.1.0, which is exactly the coordinated distribution gate docs/authenticated-ipc-rollout.md:11-14 currently describes as a manual maintainer duty, and
• gives the new jar a distinct filename.

That is a bump that buys the gate without the blast radius. If it is still deliberately declined, the reason belongs in IpcVersion's KDoc, which today reads "Bump this BEFORE merging any change to boss-ipc/src/main/proto/" and says nothing about a jar whose API broke without the proto moving. Fix this →

---

Medium

2. The recovery guard is an exception allowlist, and the regression test does not cover the call site.

repairAdviceOrNull catches six named types. The collector at KernelBootstrap.kt:497 is still a plain launch { handleFailure(...) } inside failures.collect, so anything outside that list still cancels the collector permanently - the original defect, by a different exception type. One reachable-in-principle example: orchestratorStub() reads client.channel, which builds lazily through IpcAddressResolver.configureChannelBuilder, and that throws UnsupportedOperationException for a unix:// address on a platform without kqueue or epoll. IllegalArgumentException from the same function is caught; its sibling is not.

respawn already wraps its work in catch (e: Exception), so the collector's own launch is the one place a structural guard is missing, and it would make the allowlist unnecessary rather than merely longer.

Related: RepairAdviceTest exercises the helper with a synthetic lambda. It does not go through requestRepairAdvice, so moving orchestratorStub() back above the guard - the exact bug that was reported - fails no test. The ask was for a regression "using a retained dead orchestrator handle: the failure returns null advice, and a later failure is still handled", and the ordering is the part that regressed. This repo's own AGENTS.md treats mutation-verification as the bar for a fix of this shape. Fix this →

3. A shipped native-image resource changed, and the rollout doc does not mention it. modules/boss-ipc/src/main/resources/META-INF/native-image/.../native-image.properties drops --initialize-at-build-time=io.grpc. That file ships inside the published jar, so it changes how the runtime repo's native images initialise gRPC, not only this repo's smoke binary. The comment justifies it (Netty channel factories are platform-dependent), which is convincing, but it is a downstream behaviour change that belongs in the paired-artifact checklist alongside the marker resource.

4. connectToService is a published-jar behaviour break that is also internally contradictory. if (serviceName !in serviceAddresses) return null followed by an unconditional error(...) means a function declared BossIpcClient? can never return non-null. Any runtime-side peer-to-peer caller moves from a nullable result to a thrown IllegalStateException. Whatever the resolution, this is the third API break in the jar from finding (1) and should be named in the rollout doc.

5. The bridge-test harness is now two harnesses. Three of the ten kernel/services bridge tests speak pinned TLS; the other seven (Secret, RoleManagement, Download, Git, Log, ActiveTabs, Supabase) still use ServerBuilder plus usePlaintext. The three that migrated each carry a hand-copied ~25-line block of NettyServerBuilder/sslContext/overrideAuthority wiring. boss-ipc solved exactly this with IpcTestServer; composeApp desktopTest has no equivalent, so every future bridge test re-derives the pin by copy-paste and the remaining seven have no cheap migration path.

6. EventBusServiceImpl lost observability beyond the security change. SubscriberInfo, anonIdCounter, subscriberInfo and the class's logger are all deleted. The consequence worth stating: SubscribeRequest.subscriber_id is now read nowhere at all, so a proto field that callers still set is silently dead. Deriving sourceProcess from the credential is correct; dropping subscriber bookkeeping is a separate change riding along.

---

Low / nits

7. Cross-thread ServerCall.close() race is unchanged and still undocumented. finished serialises close-against-close but not sendMessage against close: a handler thread can pass !finished.get() (ProcessIdentityInterceptor.kt:59) while the revoking thread CASes and calls super.close(). Survivable, but the next reader will assume finished made it safe.
8. Narrow admission race. When onRevoked finds the credential already gone it invokes action() synchronously at :65, closing the call - but execution continues into Contexts.interceptCall at :79, so the handler still runs and can reach the provider even though no message can escape. That contradicts the "a refused call must never reach the provider" property the bridge tests assert, in a window between :29 and :65.
9. ProcessTokenRegistry.principalFor remains @Synchronized and is called per streamed message (accepted last round; noting it persists so it is not lost).
10. ProcessTokenRegistry.issue still constructs a fresh SecureRandom() per call.
11. SpawnIpcSecurity.create:39 mints the host token from a throwaway ProcessTokenRegistry() that is immediately discarded, so the host to child credential can never be revoked for the life of that child. Probably fine, but it is not stated anywhere.
12. Terminal session ids are an existence oracle, the same shape as the state-key one that got documented. sendInput/resize/closeSession return Empty for an unknown id and PERMISSION_DENIED for a foreign one. Ids are random UUIDs so this is weak, but the state analogue was written down and this one was not.
13. TerminalServiceImpl.ownedSession opens with a bare IpcCall.current() whose result is discarded. It is load-bearing (it makes a revoked caller fail before the id lookup), which is exactly why it needs a one-line comment.
14. IpcSecuritySmoke hand-lists six test method names. A seventh added to IpcAuthorizationTest is silently not exercised natively.
15. The @JvmOverloads rationale on ProcessSpawner is still deleted. The annotation survives and KernelReflectionContractTest still guards the 3-arg shape, so nothing is broken - but a fifth parameter has now been added to that list and the reason not to drop the annotation is gone from the file.
16. modules/boss-process-manager/build.gradle.kts:39 still creates Files.createTempDirectory("bs") per test run and never deletes it.
17. TerminalOwnershipProcess.main still ignores args, and when its credential assertion fires the sentinel is never written, so the security property surfaces as a 15-second withTimeout rather than as a named failure.
18. IpcTransport.requireCompatibleRuntime never logs the value it read, so a marker mismatch and a genuinely old runtime are indistinguishable in the log. The exact-match policy itself is a settled decision; the diagnosability is not.
19. .github/workflows/authenticated-ipc.yml:5 still carries push: branches: ['fix/security-authenticated-ipc'] scaffolding. Also worth deciding before it becomes a required check: with a paths filter, a required status that does not trigger stays pending rather than passing. (I cannot edit .github/workflows/ - GitHub App permissions exclude it.)
20. docs/authenticated-ipc-rollout.md is not listed in AGENTS.md's Documentation section.

---

Things I looked at and found correct

Recording these so they are not re-litigated: ProcessSpawner puts config.environment before the BOSS_* keys and then runs IpcEnvironment.removeCredentials before installing fresh ones, so a caller cannot shadow a credential and no inherited one survives. KernelServiceImpl.registerProcess binds expectedAddress to the exact BOSS_IPC_ADDR string the host handed out, and ChildProcessBootstrap.processAddress echoes that env value verbatim, so the equality holds on Windows TCP as well as UDS. IpcTlsIdentity.restore touches no BouncyCastle, so a child needs bcpkix only if it mints an identity - worth confirming against #21's packaging, but it should not be a runtime dependency there. listStateKeys and listProcesses filter rather than refuse. IpcCall.requireOwner inside .collect gives UNAUTHENTICATED on revocation (via current()) and PERMISSION_DENIED on a foreign owner, which is what the fixtures assert.

Not verified

Upstream CI at this exact head, native Linux/macOS/Windows, the packaged host/runtime pair, and runtime #21's marker bytes and shading. Those remain the merge gates, not this read.
· branch fix/security-authenticated-ipc

[Raghav-Bansal-15]

Follow-up at 4f209d77: The authenticated artifact/minimum is now IPC 1.1.0, with the separate transport marker retained. Added the retained-dead-client request-boundary regression and non-cancellation transport fallback. Also reproduced and fixed admission-time revocation dispatching an already-refused handler. All 62 IPC tests, 4 recovery tests, quality checks and paired contract assembly pass locally; runtime #21 has 262 passing paired tests. Coordinated distribution remains a release gate.

Original and reviewer commits are preserved. Remaining current-head hosted checks and upstream approval are separate gates; no merge or release.

Native follow-up at 1ccec3f: protobuf descriptors initialize at runtime to avoid the macOS build-time initialization cycle exposed by the dependent filesystem run. Local tests/JVM smoke/quality passed. Current-head full build plus all three native IPC builds and security smoke tests now pass. Runtime #21 at 303eddb8 pins this host and passes its three-OS CI. Release coordination gates remain unchanged.

[kshivang]

@Raghav-Bansal-15 I reviewed 1ccec3f6e8d29c93ec686f0285b85be5a35306b8 against the completed Claude findings. IPC module/version 1.1.0 now names a distinct artifact and rejects the new runtime on version-aware 1.0 hosts while retaining the exact transport marker. The retained-dead-client regression calls the actual requestRepairAdvice boundary; cancellation is rethrown and other transport Exceptions fall back. RevokedAdmissionTest reproduces synchronous revocation during subscription and verifies the handler is not dispatched. Runtime descriptor initialization addresses the native build cycle; public peer-connection behavior and downstream initialization are documented.

I independently downloaded current-head fork JUnit reports: 391 relevant IPC/process/terminal/orchestrator/host-kernel tests pass per OS, zero failures/errors/skips. Full build/quality run 34983084962 and native IPC run 34983010150 pass on Linux/macOS/Windows. Read-only companion runtime #21 is at 303eddb8404f1888e380e0ffc373cedd46762f74, pins this exact host, and its three-platform run 34983216857 passes. Coordinated distribution remains a release gate; none of this authorizes publication.

No reviewer source changes or score change this pass. #669 remains an unassigned draft with unique generation/collision/rollback safeguards; no work was taken from it. Current dev merge-tree is clean. Upstream runs 34983018139 / 34983018301 need explicit workflow-execution approval. Keeping ready while that external gate is blocked, without claiming fresh Claude approval of follow-ups or requesting another review loop. No merge or release.

[kshivang]

@Raghav-Bansal-15 Your authenticated IPC and recovery follow-ups remain preserved. I reconciled this branch with current dev and the assigned process changes: #570 terminal bounds (now merged) retain authenticated per-instance ownership and credential stripping; #574 private bounded logs retain security-token cleanup on launch failure and exit. Existing terminal ownership tests now wait for bounded exit-history removal; limit tests use the authenticated transport. These integration adjustments are reviewer work, separate from your original contribution.

Head 1f4d7ddac is pushed for exact-head hosted validation. The local focused rerun is blocked behind another worker's shared build lock, so no new local pass is claimed. Original contributor-head three-platform tests were already verified. #574 remains a merge dependency while its remaining platform checks finish. No unassigned draft implementation was imported; companion runtime #21 still needs coordinated release.

[kshivang]

@claude Please review the integration changes on current head 1f4d7dd, focusing on TerminalServiceImpl/TerminalSession ownership under #570 resource bounds, ProcessSpawner credential cleanup with #574 held log streams, and the adapted authenticated terminal tests. Prior contributor findings and follow-ups were reviewed; this request is limited to the new integration, not a repeat broad rewrite.

[claude]

Claude finished @kshivang's task in 8m 14s —— View job

---

Review: integration changes at 1f4d7dd

• Isolate the four integration commits (86b1f73, a94aca7, a96602a, 1f4d7dd)
• TerminalServiceImpl / TerminalSession ownership under fix(terminal): release exited sessions without waiting for descendants #570 bounds
• ProcessSpawner credential cleanup with fix(process): bound log retention and repair native file permissions #574 held log streams
• Adapted authenticated terminal tests
• Post review

Scope and what I could verify. Limited to the integration as asked - I did not re-audit the transport, the compatibility gate or the recovery fix beyond confirming the two earlier blockers are still closed. Shallow checkout, no Gradle cache, no build permission, so nothing below was executed; findings are from reading origin/dev...HEAD and the four integration commits. Runtime #21 unread from this job. I did not edit code.

---

Blocking (a gate this head will fail)

1. TerminalLimitsTest.kt breaks ktlint's import-ordering.

a94aca7 rewrote that import block and landed io.grpc.kotlin.GrpcContextElement between io.grpc.Context and io.grpc.Status (TerminalLimitsTest.kt:17-20). Lowercase k sorts after uppercase S in the ASCII ordering the rule enforces, so io.grpc.kotlin.* has to come after io.grpc.StatusException.

Three things make this mechanically checkable rather than a guess:

• .editorconfig disables eight standard rules by name and import-ordering is not among them.
• build.gradle.kts:44 applies ktlint to allprojects with ignoreFailures.set(false), wired into check, so :boss-app-terminal:ktlintTestSourceSetCheck runs in the code-quality job.
• The only two other files in the tree importing io.grpc.kotlin.* both order it correctly: ProcessIdentityInterceptor.kt:11-12 (Status then kotlin.CoroutineContextServerInterceptor) and OrchestratorServiceImplTest.kt:12-13.

The PR body reports "desktop ktlint/detekt pass", which is :composeApp, not this module - and the earlier shared-lock run that passed root ktlintCheck was at 45edecf8c, before this file was touched. ./gradlew :boss-app-terminal:ktlintFormat fixes it. Fix this →

---

Medium

2. closeSession takes two calls to remove a session, and the adapted test encodes that as a poll loop.

TerminalServiceImpl.kt:170 is if (session.active) session.terminate() else synchronized(lock) { sessions.remove(session.id) }. Since active only flips false in the pump thread's finally after process.onExit().join(), one closeSession never both stops and removes. TerminalOwnershipTest.closeAndRemoveHistory (:140-150) is the consequence: close, then spin on listSessions re-issuing closeSession every 10ms.

The ownership interaction is the part worth deciding rather than accepting. session() requires IpcCall.requireOwner, so the second call needs a still-valid credential - and the test reusing a process id cannot acquire the previous instances terminal is exactly the case where the original owner's credential is gone. Its retained sessions can then only be removed by HOST or by retain eviction. That is bounded, but it means an ordinary plugin restart leaves its exited terminals in the map with no caller able to clear them.

Setting a purgeOnExit flag in closeSession and removing from onStopped would make one call sufficient, delete the poll loop, and remove the need for a live credential at the moment of exit. The loop is also a latent NOT_FOUND: if an entry is evicted by retain between the listSessions check and the next closeSession, session() throws NOT_FOUND out of the helper. Not reachable in these two tests (nothing creates sessions concurrently), but it is reachable in anything shaped like TerminalLimitsTest's historyLimit = 2. Fix this →

3. retain can exceed historyLimit when activeLimit == historyLimit. init allows equality (require(activeLimit > 0 && historyLimit >= activeLimit)), and retain (:102-108) only evicts entries whose active is false. With both limits equal and every slot active, the eviction loop exhausts its iterator without removing anything and the put still lands, so the map reaches historyLimit + 1. Bounded by activeLimit + 1, so not a leak - but the documented "completed-session retention" bound silently does not hold for that configuration. Either require(historyLimit > activeLimit) or evict after insert.

4. Terminal ownership has no written semantics, and the state-key analogue does. docs/authenticated-ipc-rollout.md:23-27 deliberately writes down that state key existence is not confidential. The terminal surface now has the same shape and more: sessions are owner-instance scoped; ProcessAuthority.HOST administers and SUPERVISOR deliberately does not; exit history is retained until an explicit second close; and session() answers NOT_FOUND for an unknown id versus PERMISSION_DENIED for a foreign one, which is a session-id oracle (weak - random UUIDs - but the state one was weaker and still got written down). Adding a short paragraph beside the state-key section is the cheap version.

Relatedly, docs/authenticated-ipc-rollout.md is still not listed in AGENTS.md's Documentation section, and the new "Process log authority and lifetime" section says nothing about the credential handoff those logs now sit beside.

5. TerminalOwnershipTest.cleanup never calls service.close(), unlike TerminalLimitsTest. It closes each listed session through the host stub (:46-50), which for an active session is terminate() and returns immediately, then deletes the temp root. Two consequences: nothing awaits child exit before root.toFile().deleteRecursively(), so on Windows the delete silently fails while a killed child still holds the directory as its cwd; and if a test fails before reaching its own cleanup of a session, the child JVM outlives the test class with its activeSlots permit held. TerminalLimitsTest.cleanup gets both right precisely because service.close() waits up to five seconds for each active process.

6. SecureSpawnTest reads a log file eagerly as an assertion message. SecureSpawnTest.kt:53-56 passes Files.readString(logs.resolve("secure/stderr.log")) as the String message argument of assertTrue, so it is evaluated on every run including the passing one. RotatingProcessLog.rotate creates stderr.log lazily on the first append, so the file exists only because the fixture JVM writes something to stderr - and boss-process-manager has no SLF4J binding in its test dependencies, so what guarantees it today is SLF4J's own "no providers" warning. Add a binding, or silence that warning, and the test fails with NoSuchFileException instead of the assertion it was diagnosing. The assertTrue(message) { block } overload plus a Files.exists guard makes it a real diagnostic. Fix this →

---

Low

7. SpawnIpcSecurity.create can leak a live client. It builds BossIpcClient at :46 and only then calls registry.issue(...) at :54. If issue throws, create never returns, so security is still null in ProcessSpawner and onFailure's security?.revoke() is a no-op while the channel stays open. Carried forward from the earlier nit, now more visible because logs.close() sits in that same onFailure and is the half that does work.
8. ProcessSpawner's in-block comment no longer points at the code it justifies. "Minted after config.environment, so nothing a caller supplies can shadow the real credential" now sits at :82-83, immediately after IpcEnvironment.removeCredentials, while the actual mint moved to security?.install(...) at :91. The ordering it describes is still correct; the comment is three lines from the wrong statement. Its KDoc also still lists only the three BOSS_* variables and not the five credential variables the child now receives.
9. The child's HOST_TOKEN is unrevocable and still unstated. SpawnIpcSecurity.create:39 mints it from a ProcessTokenRegistry() that is discarded on the next line, so revoke() cannot invalidate the host-to-child direction - only the child's own death does. Defensible (the accepting registry dies with the child), but this is the one credential in the handoff that per-incarnation revocation does not cover, and IpcEnvironment's restored threat-model paragraph does not say so.
10. An interrupted onExit().join() leaves a stream permanently open. TerminalSession.startPump's finally appends the exit chunk only after process.onExit().join().exitValue(). If that throws (thread interrupt), active still flips and the slot is still released - correct - but TerminalOutputBuffer.finished stays false, so every streamOutput collector on that session suspends forever in updates.first { } holding one of the 32 streamSlots. Admission is safe; reader permits are not.
11. resize does not check active, so resizing a session retained in exit history succeeds silently where send answers FAILED_PRECONDITION.
12. TerminalOwnershipProcess.main still ignores args, so request("wait"), request("echo") and request("sentinel") are the same program, and a fired credential assertion still surfaces as the 15-second sentinel poll rather than as a named failure. Carried forward.
13. modules/boss-process-manager/build.gradle.kts:44 still creates Files.createTempDirectory("bs") per test run and never deletes it. Carried forward.

---

Looked at and found correct

Recording these so they are not re-litigated.

• Credential ordering survived both merges intact. ProcessSpawner is putAll(config.environment) then the four host BOSS_* puts then IpcEnvironment.removeCredentials then security.install - so a caller can shadow neither the address keys nor a credential, and nothing inherited survives. TerminalSession.launch:155-164 applies the same discipline in the opposite direction (putAll(request.environmentMap) then strip), with the comment stating why the strip is last, and TerminalOwnershipProcess proves it by inspecting its own environment in a real spawned JVM and throwing on any of the five names.
• IpcCall.requireOwner(ownerInstance) is the last statement before builder.start(), so a credential revoked mid-launch refuses the spawn rather than orphaning a process. The ownership read happens once at createSession:36 and is carried immutably onto the session, so a later revocation cannot retroactively change who owns it.
• Admission accounting is right on every path. activeSlots.release() is unconditional in the !pumping branch of the finally, so a launch that threw releases; the pump thread's onStopped releases on the pumping path; and cancellation during return dispatch terminates the unclaimed process drives exactly that split through a paused dispatcher.
• fix(terminal): release exited sessions without waiting for descendants #570's property is preserved. The pump never blocks on EOF (available() polling while alive, then a bounded post-death snapshot) and admission is released on parent exit rather than descendant EOF, which an exited shell releases capacity while a descendant still holds stdout covers inside a five-second bound.
• ipcClient becoming null for an unauthenticated spawner is handled at all three consumers, not just the one this PR touched: checkNotNull at OutOfProcessPluginSpawnerImpl.kt:147, null-return at KernelBootstrap.adviserStub, and the null guard in ProcessRegistryCapabilityResolver. @JvmOverloads still yields the 1-arg and 3-arg shapes ReapChildrenTest and ProcessSpawnerRegistrationTest use reflectively.
• Log lease refcounting is balanced across both paths. acquire adds two references and hands out two leases; either the two drain threads close them, or logs.close() does, and Lease.close is idempotent under its own monitor - so the partially-attached failure path cannot double-decrement.
• The test migration is honest. TerminalLimitsTest routes every call through the authenticated stub except the one paused-dispatcher test, which needs a direct in-process call and supplies identity via GrpcContextElement over the same token, so stub.listSessions still observes that session. TerminalOwnershipTest's status split is right: UNAUTHENTICATED for an unrecognized or superseded credential, PERMISSION_DENIED for an authenticated non-owner, including inside .collect.
• Both earlier blockers remain closed. repairAdviceOrNull now catches Exception (not a six-type allowlist) and rethrows CancellationException first, and adviserStub is called inside the lambda with a comment saying why.

Carried forward, not re-argued

launch { handleFailure(...) } at KernelBootstrap.kt:497 still has no structural guard - now much narrower, since the fallible part is inside repairAdviceOrNull and respawn catches its own. ProcessTokenRegistry.principalFor is still @Synchronized and called per streamed message, which terminal output streaming is the heaviest user of. The cross-thread ServerCall.close() versus sendMessage race is unchanged and still uncommented.

Not verified

Exact-head upstream CI, the three-OS native builds, the packaged host/runtime pair, and runtime #21's marker bytes and shading.
· branch fix/security-authenticated-ipc

[kshivang]

Integration review disposition:

• ⬆️ actions:(deps): Bump supabase/setup-cli from 2 to 3 #1 formatting/import order fixed in 7dfc491; exact-head code-quality passed.
• ci: use GitHub-hosted arm64 runners instead of self-hosted labels #5/security: stop DigiCert signing secrets leaking into public CI logs #6 fixed in 45697cf: ownership cleanup now calls service.close() before client/server teardown and root deletion; secure-spawn stderr diagnostics execute only after readiness failure and tolerate missing logs.
• security: gate release workflows behind owner-approved environment #4/ci: commit automated version bumps as a human identity, not a bot #9: rollout docs now state terminal owner/HOST/SUPERVISOR semantics, bounded retained exit history, ID-existence status distinction, and the child-lifetime host-controller credential. Linked from AGENTS.
• ⬆️ actions:(deps): Bump actions/setup-java from 4 to 5 #2: retained exit history is the existing fix(terminal): release exited sessions without waiting for descendants #570 contract, bounded and inaccessible to replacement instances. HOST cleanup and admission eviction remain available. The helper has no concurrent session creation/eviction in either test; changing close semantics here would be a separate behavioral change.
• ⬆️ actions:(deps): Bump actions/setup-go from 6 to 7 #3 is not reachable as described: reserveSlot() acquires before retain() under the same service lock. With all active slots occupied, admission fails before insertion; a released slot is published only after active=false, so an evictable entry exists when historyLimit == activeLimit.
• docs: add a centered hero to the README opening #10: CompletableFuture.join() is not interruptible like get(); an interrupt does not throw InterruptedException there. No demonstrated production exceptional onExit future was supplied. Existing terminal pump/exit regressions remain in validation.
• docs: document Toolbox and MCP in README; fix open-source staleness #7: registry.issue has no argument-rejection path and revocation listeners are individually runCatching; no ordinary recoverable throw was demonstrated. Not adding speculative resource restructuring for fatal allocation/provider failures in this bounded integration pass.
• Remaining low items (comment placement, retained-session resize, fixture mode labels, short test temp directory) are follow-ups rather than security/merge blockers.

The new head includes only these focused test/doc repairs after reviewed integration; exact-head CI is required again. No fresh review loop requested. Original author score remains unchanged; integration repairs are reviewer work.