[Antriksh1984]

Summary

Require verified caller identity for all six RunConfiguration RPCs. Execute resolves the requested nonblank id against the provider-held detected configuration and discards caller-supplied command, arguments, directory and environment. Streams capture identity synchronously and revalidate before each emission; idle revoked streams are not proactively disconnected.

A stale Run during/after rescan is refused without executing a replacement configuration. The IPC proxy reports execution failures through the existing run-panel error banner, preserves local errors across host snapshots, and dismisses them on Clear or successful retry. Cancellation propagates from Execute, Scan and Clear. Failure text does not expose remote exception payloads or incorrectly assert that every failure is a stale id.

Credit

Antriksh (@Antriksh1984) authored the original identity gates, provider-held execution design and four original wire tests; also authored the later client-visible lastError/cancellation repair and two proxy tests. Original author commits remain intact.

Previous reviewers added unary/live-stream revocation tests, audit/refusal diagnostics, blank-id refusal, rescan and forwarded-argument tests, and documentation repairs. Prior reviewers added separate local/host error state, dismissal/retry handling, generic failure wording, and seven proxy regressions. These are agent repairs, not original contributor score credit.

Validation and scope

Contributor replay 4550e5f passed all ten hosted checks. Review integration 940cf69 merges dev 5a2e818 while retaining current baseline documentation. Follow-up agent repair 8cd4fe1 fixes newer scan-error precedence, clears run feedback at scan start, neutralizes failure wording, adds caller-only scan audit and strengthens regressions. Final local validation passes 8 bridge + 12 proxy tests, plus ktlintCheck/detekt in both affected modules. All ten hosted checks now pass on final head 8cd4fe1 (run 34838505127); downloaded Linux XML confirms 8 bridge + 12 proxy tests with no failures, errors or skips. Latest Claude findings were reconciled against the repair; current dev browser swipe changes do not touch the bridge, adapter, dependencies or build configuration. These follow-up repairs are agent work, excluded from the original contributor score.

Authorization belongs at the host bridge; client error handling belongs in the published IPC adapter. No plugin API/proto change, runtime publication, app run or live database change. Shared transport/helper/runtime compatibility work (#575) and Windows run disambiguation (#653) remain separate. No UI layout change; no native visual exercise claimed.

[kshivang]

@claude Please review current head 8253c236180a79fa9b9cd22d13dccc9c0559aeb6 against dev for correctness, security and regressions. Please check every RPC identity gate, stream revocation/context capture, provider compatibility and regression coverage. Current-head hosted CI is green. This is the single review request for this sweep.

[claude]

Claude finished @kshivang's task in 5m 12s —— View job

---

Review of 8253c23 vs origin/dev

• Read the diff vs origin/dev
• Check every RPC's identity gate
• Check stream revocation / context capture
• Check provider compatibility (RunConfigurationDataProvider, proto, proxy, server wiring)
• Compare against the sibling Remote UI: StreamUI has no plugin identity check — one plugin can bind another's surface #53 bridges for pattern drift
• Assess regression coverage in RunConfigServiceBridgeTest

Verdict: the security change is correct and the design choice is the right one. No blocking defect found. Six findings below, one of which (the ephemeral-id race) is a real behavioural regression risk, and one of which (no revocation test) means the feature the PR description leads with is untested.

I did not run Gradle here; taking current-head hosted CI as green per your note.

---

What I verified holds

Every RPC is gated. run_config.proto:13-30 declares exactly six RPCs; all six are overridden and all six refuse without an identity. Nothing falls through.

Stream context capture is right, and for the documented reason. CURRENT_IDENTITY.get() is read synchronously in the RPC method body, before the flow { } is ever collected — the same shape and the same rationale as PluginUIServiceBridge.streamUI (PluginUIServiceBridge.kt:153-159: the interceptor's Context is not carried into the coroutine that later runs the flow). Capturing the supplier rather than the resolved id is what makes the per-emission re-check possible; the closure holds token + registry, so it answers correctly on whatever thread it lands on. Per-emission, the check runs before emit, so no value escapes after revocation.

Fail-closed is complete. Absent interceptor → CURRENT_IDENTITY.get() is null → every stream refuses; AUTHENTICATED_PROCESS_ID.get() null → every unary refuses. RunConfigServiceBridge is only ever registered on the one server built with a real registry (KernelBootstrap.kt:486 → :987), so that is not a live outage risk.

Provider compatibility is fine. detectedConfigurations is a StateFlow on the API interface, so .value at RunConfigServiceBridge.kt:124 is sound for all three implementations (RunConfigurationDataProviderImpl, RunConfigDataProviderProxy, the test fake). The client rides the same kernel channel that already carries ProcessTokenClientInterceptor (ChildProcessBootstrap.kt:85) — the one SecretDataProviderProxy has depended on since #505, so no new wire surface.

Deleting toData() rather than leaving it unused is the better half of the fix. The proto→data conversion is the trust path; removing it means a future caller cannot reintroduce it by reaching for a function that is sitting right there.

---

Findings

1. [coverage] The per-emission revocation check is untested — delete it and the suite still passes.
The PR description leads with "the three watch streams re-check identity per emission so a revoked token stops a live stream, not just new calls," and nothing in RunConfigServiceBridgeTest exercises it. There is no revocation test of any kind here, unary or streaming, while both siblings have one (RoleManagementServiceBridgeTest.kt:230, and SecretServiceBridgeTest). Mutating away lines 53-55 / 73-75 / 88-90 leaves all four tests green. Concretely: issue a revocable token, open watchIsScanning, assert the first emission arrives, tokenRegistry.revoke(...), flip _isScanning, assert the stream terminates with PERMISSION_DENIED. That is the single highest-value test to add, because it is the only novel mechanism in the diff.
Fix this →

2. [correctness — behaviour] Ephemeral ids plus a scan that empties the list make a refused Execute a silent no-op.
Detected ids are UUID.randomUUID() minted fresh on every scan (DesktopMainFunctionDetector.kt:136), and scanProject sets _detectedConfigurations.value = emptyList() for the whole duration of the scan (DesktopRunConfigurationManager.kt:132). So an Execute arriving during or just after a rescan resolves nothing and is refused — and RunConfigDataProviderProxy.execute swallows every exception (RunConfigDataProviderProxy.kt:129, catch (_: Exception) {}), so neither the plugin nor the user sees anything: Run does nothing, with the only trace a warn in the host log. Before this PR it ran.

The ordering limits the blast radius and is worth stating: the plugin's list comes from the same mirror the bridge reads, so the plugin can never be ahead of it. This is a race (user clicks Run across a project switch / rescan), not a systematic break. Two mitigations that both preserve the security property, since both still execute the provider's own copy:

• distinguish "unknown id" from "the provider is mid-scan" (see finding 3) so the client can retry or say why;
• on a miss, resolve by (filePath, lineNumber, command) against provider.detectedConfigurations.value as a fallback — matching on a stable tuple instead of a per-scan UUID.

3. [correctness — minor] PERMISSION_DENIED is the wrong code for an unknown id, and the log omits the id.
RunConfigServiceBridge.kt:131 — an id the provider does not hold is not an authorization failure; the caller was authorized. NOT_FOUND (or FAILED_PRECONDITION while scanning) reads truer and is exactly the distinction finding 2 needs on the client side. Relatedly, the refusal at :126-130 logs caller but not requestedId, so an operator debugging the silent no-op cannot tell which id missed. The id is a UUID, not sensitive — safe to log.

4. [doc accuracy] The KDoc claims a set the provider does not contain.
RunConfigServiceBridge.kt:34-36 says the lookup covers "the auto-detected and operator-saved configurations." It does not: RunConfigurationDataProviderImpl.kt:38 mirrors only RunConfigurationManager.detectedConfigurations, i.e. scan output. Operator-saved configurations live in currentSettings.configurations and never reach this provider. The host's own run path checks saved first, then detected (BossAppDialogs.kt:589-593), so the doc overstates by exactly the half the host consults first. Nothing in-repo breaks, because plugins can only ever see detected ones — but the sentence should say so, and the asymmetry with the host's own lookup is worth a line as known-and-accepted (closing it needs a plugin-API change, not a change here).

5. [consistency] Two KDocs still name three fail-closed bridges; there are now four.
ProcessIdentityInterceptor.kt:21-24 ("the three that currently make it") and BossIpcServer.kt:37-42. The second is an operational warning — "Any kernel server hosting one of those bridges must be constructed with a real registry" — so it being stale is the one that could actually mislead someone wiring a new server. (PluginUIServiceBridge is in the first list but missing from the second already.)

6. [consistency] No attributed audit line on a successful Execute.
RoleManagementServiceBridge.logMutation (:208-214) records every attributed mutation. Here the sharpest verb in the tree — the one that opens a terminal and runs a command — logs only refusals. An info line with caller + config id + name on success is the record you would want after an incident, and completes the #505 pattern this PR is following.

---

Non-blocking notes

• Three copies of the identity preamble. Lines 46-64 / 66-79 / 81-94 are the same eight lines three times, including the revocation rule. This repo's own AGENTS.md repeatedly records predicates drifting once they exist in two places. A private identityGuardedFlow { } helper would leave one definition.
• scanProject's path is still unconfined (pre-existing, not introduced here). An authenticated plugin can scan any absolute path and read the resulting absolute file paths and generated commands back off WatchDetectedConfigurations, and doing so clobbers the operator's detected list. Contrast projectSearchProvider, which AGENTS.md documents as confined to the open project, canonical and symlink-checked. windowId is likewise still caller-chosen on both scanProject and execute. Identity gating does not address either; worth recording on the Remote UI: StreamUI has no plugin identity check — one plugin can bind another's surface #53 follow-up rather than expanding this PR.
• The provider.calls.isEmpty() assertion is vacuous for the three watch RPCs (the fake records only scanProject/execute/clearError). The stream refusals still have teeth via assertFailsWith on take(1), so this is cosmetic — but watchIsScanning/watchLastError have no success test, only a refusal one.
  · branch fix/run-config-caller-identity

[kshivang]

@Antriksh1984 Thank you for the concrete IPC security work. Current repair head: 9f29c91f9dafe445c666cb46d5b94c06f871303c.

Your host-held configuration lookup closes the untrusted command payload path and retains all six RPC identity gates. Reviewer commit adds unary and mid-stream revocation regressions, logs stream refusals, records execution requests using caller plus provider-held configuration id, records scan state on lookup misses, and corrects the KDoc to say auto-detected configurations only. Operator-saved configurations are not in this provider.

The real remaining behavior risk is a Run click across a rescan: scanning empties the provider list and generates new UUIDs, while RunConfigDataProviderProxy.execute swallows the refusal. I did not add the suggested request-field tuple fallback: it does not solve the empty-list interval, and selecting a replacement execution from caller-supplied fields needs an explicit stale/project identity contract. The current exact-id allowlist therefore remains fail-closed. Likewise, changing PERMISSION_DENIED to NOT_FOUND alone does not repair a client that swallows both; retain the existing allowlist refusal contract until client behavior is coordinated. The new host log identifies whether a miss happened during scanning without logging arbitrary request payloads.

Next steps: make stale/unknown configuration refusal visible to the run client (or establish a safe project-scoped refresh/retry contract), and add a test for Execute during/after rescan proving that no caller-supplied command executes and that the user receives a useful result. Cover successful known-config forwarding, unknown ids, and both new revocation tests. The proxy/runtime follow-up is cross-repository context, not taken over by this assignment.

The latest requested Claude review is #634 (comment). Shared authentication-helper consolidation and stale transport inventory comments remain #575 context; no cross-group PR was changed. Avoid introducing another generic flow abstraction in this bounded security patch: the existing synchronous capture sites remain explicit, and refusal logging is centralized locally.

Original-head CI and actual XML were verified green on Linux, macOS and Windows. That evidence does not validate these new reviewer changes. Updated-head CI is queued/in progress; local Gradle validation could not acquire the shared build lock during this bounded review. This is an infrastructure/validation handoff, not evidence of an original contributor CI failure. All repairs are reviewer work and are not counted toward your original contribution score. No app instance or live database was launched or stopped.

Per the assigned bounded-disposition policy, I am converting this PR to draft after this handoff. No checks were bypassed and no unvalidated merge was attempted.

[Antriksh1984]

Confirmed current head 9f29c91f (with the revocation tests, execution audit log, and NOT_FOUND/scan-state distinctions from the review) is 10/10 green and mergeStateStatus: CLEAN against dev. Restoring ready-for-review.

[kshivang]

@claude Please review current head 475b88cb1b77c51d8c91c85396e4179c181f94d3 against dev for correctness, security, regressions and relevant test coverage. Only this PR’s own commits were replayed onto current dev to remove squash-history conflicts; original contributor authorship is preserved. Prior review repairs are retained. Please recheck stale-id/rescan behavior and actual client error handling; the current code still returns PERMISSION_DENIED for an unknown id. This is the single current-head review request for this sweep. New-head CI is pending.

[claude]

Claude finished @kshivang's task in 5m 27s —— View job

---

Review of 475b88c vs origin/dev

• Read the diff vs origin/dev
• Confirm the replay preserved the reviewer repairs
• Re-check every RPC identity gate
• Re-check stream context capture / per-emission revocation
• Re-check stale-id / rescan behaviour and the actual client error handling
• Assess test coverage at this head

Verdict: the replay is clean, every prior repair survived it, and I found no blocking defect. The security property holds at this head. One new finding below (a blank-id hole in the allowlist, with in-repo precedent for the guard); the rest is the stale-id/client answer you asked for, plus coverage and doc drift.

I could not run Gradle in this job (the sandbox refused the ./gradlew invocation), so everything below is from reading the code. New-head CI is still the validation of record.

---

Replay verification

git log origin/dev..HEAD is exactly three commits (db572a4, d0419f9, 475b88c), origin/dev is 8e05bf6 and is an ancestor, and the diff is the two expected files at +413/-38. 9f29c91 is not in this checkout, so I verified the repairs by content rather than by SHA comparison - all four are present:

Repair	Where
unary + mid-stream revocation regressions	RunConfigServiceBridgeTest.kt:179, :188
stream refusals logged	RunConfigServiceBridge.kt:152-159 (refuseIdentity)
attributed execute audit line	:106-110, caller + provider-held config.id
scan state recorded on a lookup miss	:135 (isScanning)
KDoc corrected to auto-detected only	class KDoc, "operator-saved configurations are not exposed by this provider"

What I re-verified holds

All six RPCs are gated, and the stream capture is correct. run_config.proto declares six; all six refuse without an identity. CURRENT_IDENTITY.get() is read in the RPC method body before the flow { } is collected, and the closure holds the supplier, which is what makes the per-emission re-check possible. The check runs before emit, so no value escapes after revocation. ?: refuseIdentity(...) returning Nothing is what smart-casts currentIdentity non-null for the emission check - so unlike DownloadServiceBridge.kt:47's currentIdentity?.invoke(), the null branch here is provably unreachable rather than defensively re-tested. Correct, and slightly tighter than the sibling.

Fail-closed is complete and not an outage risk. No interceptor → null identity → every RPC refuses; the bridge is registered only on the one server built with a real registry (KernelBootstrap.kt:987).

The allowlist and the client's list are the same StateFlow, which is the property that keeps them consistent. I chased this further than last time because execute now depends on a mirror it previously never read. RunConfigurationDataProviderImpl.kt:37-41 mirrors the global RunConfigurationManager on Dispatchers.Main; watchDetectedConfigurations serves that same mirror. So the client can never hold an id the allowlist has not held - and if that mirror collector ever stalled, execute would refuse everything and the client's list would be empty too, which is at least self-consistent rather than a phantom-id state. The provider is by lazy per DefaultPlugin (per window), but RunConfigurationManager is a singleton, so multiple windows do not divide the allowlist.

Revocation is defence-in-depth, not a live path. revoke is only called from ProcessSpawner.kt:105-107 (child exit) and KernelBootstrap.kt:184 (reap), so a running client's token is not revoked out from under it. The tests are still the right tests; nobody should "optimise" the per-emission check away on the grounds that it never fires in production.

---

Findings

1. [correctness] A blank requested id can match, and the sibling bridge guards exactly this.
RunConfigServiceBridge.kt:130 is find { it.id == requestedId } with no blankness guard on either side. ExecuteConfigRequest.configuration is an unset message for a caller that sends ExecuteConfigRequest.newBuilder().build(), so request.configuration.id is "" - and that matches any provider entry whose id is blank. Nothing in today's detector produces one (DesktopMainFunctionDetector mints UUIDs), so this is not exploitable at this head; it is an allowlist that silently degrades if any future provider, adapter or hand-built RunConfigurationData ever carries a blank id, and the caller does not even have to guess anything to hit it. DownloadServiceBridge.kt:139-148 guards precisely this shape on both sides (requestedPath.takeIf { it.isNotBlank() } and item.destinationPath.isNotBlank()), which is the in-repo precedent this bridge is otherwise following line for line. Two lines, same reasoning, closes it permanently.
Fix this →

2. [answer] PERMISSION_DENIED for an unknown id: keeping it is the consistent call, but the caveat should be written where the refusal is.
You asked me to recheck this. My previous review argued for NOT_FOUND; having now read the sibling that landed on dev, I think keeping PERMISSION_DENIED is the better choice here, because DownloadServiceBridge.kt:154 already returns exactly that for the same class of allowlist miss ("this path is not a tracked download"). Two bridges answering the same question two ways is worse than one arguably-imprecise code, and changing it should change the family together.

What the sibling also does, and this one does not, is document the race at the refusal site: DownloadServiceBridge.kt:130-132 says "The provider snapshot is sampled for UI delivery; a newly created download remains refused until that snapshot includes it. Callers should use paths observed from watchDownloads." That sentence is exactly the rescan story here - DesktopRunConfigurationManager.scanProject sets _detectedConfigurations.value = emptyList() for the duration of the scan and the detector mints fresh UUIDs after it - and knownConfigurationOrRefuse's KDoc (:121-125) does not say it. One sentence, and the two bridges tell the same story to the next reader.
Fix this →

3. [answer] The client swallow is in this repo, so only half of the follow-up is cross-repository.
Your handoff called the proxy/runtime follow-up cross-repo context. The file that eats the refusal is here: plugin-platform/plugin-api-ipc/.../RunConfigDataProviderProxy.kt:129 (catch (_: Exception) {} on execute, and the same at :113 scanProject and :136 clearError). Nothing in this repo constructs RunConfigDataProviderProxy - I grepped; the class is a published library the out-of-repo runtime wires - but the swallow itself is repairable here without touching the other repo. What genuinely needs cross-repo coordination is only the plugin-facing contract, since RunConfigurationDataProvider.execute returns Unit and throwing from it changes the pinned API's behaviour.

Two concrete details for that follow-up:

• The three watch loops (:44-99) treat every status as transient and reconnect with 1s→30s backoff forever. For a refusal that will never become allowed, that is a permanent loop - benign today only because revocation coincides with child teardown (see above), so the loop dies with the process. Anyone hardening this should treat PERMISSION_DENIED/UNAUTHENTICATED as terminal rather than retryable.
• The cheapest visible-failure repair that needs no API change is to set _lastError on a refused execute, since the run panel already renders lastError. Caveat worth knowing before doing it: _lastError is also driven by watchLastError, so a host-side change would clobber a locally written message - which is fine for the revoked case (the stream is refused too, so nothing overwrites) and lossy for the stale-id case.

4. [coverage] Neither scanProject nor execute has its forwarded arguments pinned.
FakeRunConfigurationDataProvider records only the method name for scanProject (:275), and executed captures the config but never the windowId. So the suite would stay green if scanProject forwarded a wrong path, dropped windowId, or execute sent the wrong window - and windowId is caller-chosen on both RPCs. Recording projectPath/windowId in the fake and asserting them is a few lines and closes the one part of the request this bridge does still pass through untouched.
Fix this →

5. [coverage] The rescan regression you asked for next is mechanically covered but behaviourally undocumented.
"Execute during/after a rescan" and "unknown id" reach the same branch through the same predicate, so RunConfigServiceBridgeTest.kt:159 already proves no caller-supplied command runs in that window - the security half of your next step is done. What is missing is the behavioural assertion: observe a config, empty the list as a scan does, execute its id, assert the refusal and that nothing ran. Worth adding as the record of an accepted behaviour rather than as new coverage of a new mechanism, and it will pin the contract if anyone later adds a tuple fallback.

6. [coverage] watchIsScanning and watchLastError have no success assertion on their values.
The revocation test proves a first emission arrives on watchIsScanning but asserts nothing about it, and watchLastError is only ever refused. That leaves error ?: "" (:91) - the only non-trivial mapping in the three watches, and a lossy one, since a client cannot distinguish "no error" from "an empty error string" - untested in both directions.

---

Non-blocking notes

• Refusal log wording differs from the other five bridges. :155 says "no current verified process identity on this call"; Secret, Role, Supabase, Download and PluginUI all say "no verified process identity on this call". This bridge is the only one that logs stream refusals at all, which is an improvement worth keeping - but aligning the string means an operator can grep one phrase across all six.
• Sixth private copy of authenticatedCallerOrRefuse + NO_IDENTITY. Noted only; you explicitly ruled out an abstraction in this bounded patch and I agree with that call for this PR. The five on dev are already identical, so this is a family-wide cleanup or nothing.
• Two shared-module KDocs now understate the gated set by half. ProcessIdentityInterceptor.kt:21-24 still says "the three that currently make it" and BossIpcServer.kt:33-40 names only Secret and RoleManagement in what is an operational warning ("Any kernel server hosting one of those bridges must be constructed with a real registry"). The real count is six: Download, PluginUI, RoleManagement, RunConfig, Secret, Supabase. This drift predates this PR (Download and Supabase landed without updating it) and fixing it edits modules/boss-ipc, which widens this PR's blast radius - reasonable to defer, but it is the stale doc most likely to mislead whoever wires the next server.
• scanProject's path and both RPCs' windowId remain unconfined (pre-existing, unchanged here). An authenticated caller can still scan any absolute path, read the resulting absolute file paths and generated commands back off WatchDetectedConfigurations, and clobber the operator's detected list - and directing a known configuration's execution at an arbitrary windowId is still unvalidated. Identity gating does not address either; Remote UI: StreamUI has no plugin identity check — one plugin can bind another's surface #53 follow-up territory, not this PR's.
• provider.calls.isEmpty() at :109 is still vacuous for the three watch RPCs (the fake cannot record a property read). The stream refusals keep their teeth via assertFailsWith on take(1), so this is cosmetic.
  · branch fix/run-config-caller-identity

[kshivang]

@Antriksh1984 Your provider-held exact-id execution closes the untrusted command payload path, and all six RPC gates and prior revocation repairs survived the clean dev replay. I verified your update: the prior repair head did pass all ten checks and six focused tests on every supported OS. That earlier validation blocker was resolved, not an author failure.

Current head 3ecf71e2ceff800125b2c716bc6b5da5ebee24d9 additionally rejects blank/whitespace ids, explicitly tests refusal of a previously observed id during and after a rescan, and pins scan path/window and execution window forwarding. These are reviewer repairs, with original authorship preserved. The current Claude review agrees the security gate is correct and that PERMISSION_DENIED is consistent with the Download allowlist; it supersedes the earlier NOT_FOUND suggestion. The contributor comment's NOT_FOUND claim was not present in the actual source. The local pre-follow-up six-test suite/ktlint/detekt passed; the final eight-test head is being revalidated after correcting two reviewer-added long test lines.

Remaining substantive integration issue: DesktopRunConfigurationManager clears the detected list and remints UUIDs on rescan; plugin-platform/plugin-api-ipc/RunConfigDataProviderProxy.execute catches every exception and returns Unit. A stale Run click therefore silently does nothing. The proxy source is in this repository; only its published runtime consumption is cross-repository. Please expose a durable client-visible stale/unknown execution error (avoid watchLastError immediately overwriting it), or define an explicit safe project-scoped refresh/retry contract. Add proxy/UI integration tests for during-scan and post-scan stale clicks, no caller-command execution, visible failure, recovery on selecting a fresh id, and cancellation propagation. Do not silently resolve a replacement from caller-controlled command/path fields.

Shared transport/helper inventory remains #575 context. Extra watch value/subscription assertions are follow-up test strength, not a claim that the current gates leak. No runtime release, native UI exercise or live database operation was attempted. I am converting this PR to draft for the concrete client behavior above and current-head validation; pending CI is not an original contributor defect. Original score remains 8.5/10 and excludes reviewer repairs.

[kshivang]

Final validation update: 31f41a94b now passes all eight local RunConfigServiceBridge tests (zero failures/errors/skips), ktlint and detekt. The reviewer-added formatting violations are corrected. The draft handoff remains for the client-visible stale-Run behavior; no further Claude request is being sent.

[Antriksh1984]

Rebuilt this PR's branch cleanly onto current dev (the prior head had accumulated unrelated drift - GitServiceBridge/LogServiceBridge, the now-merged encryption migration, etc.) and closed the remaining "client-visible stale-Run" item from the last review:

RunConfigDataProviderProxy.execute caught every exception from a refused Execute and discarded it silently - so a Run click during or just after a rescan (the window where detectedConfigurations is momentarily empty and ids are freshly minted) resolved to nothing server-side, and the client told the operator nothing: no terminal, no error, only a warn line in the host log. It now sets lastError on a refused call, which the run panel already renders - the one client-visible signal RunConfigurationDataProvider has, so no plugin-API change was needed. Also stopped that same catch from swallowing CancellationException.

Added RunConfigDataProviderProxyTest (2 cases, against a fake service so the auth boundary stays RunConfigServiceBridgeTest's to cover): a refused execute leaves lastError non-null, a successful one leaves it untouched.

Current head dac77a00: compiled, detekt clean, and RunConfigServiceBridgeTest (8) + RunConfigDataProviderProxyTest (2) pass locally - 10/10. Hosted CI running now.

[kshivang]

@claude Please review current head dac77a0 against current dev, including contributor updates since the previous handoff. Verify retained identity/revocation protections, regressions and tests. Specifically check the new proxy lastError behavior under real watchLastError emissions, cancellation and non-stale RPC failures.

[claude]

Claude finished @kshivang's task in 5m 23s —— View job

---

Review of dac77a0 vs dev

• Read the diff (this PR's two commits)
• Re-verify the six RPC identity gates and stream capture
• Re-verify per-emission revocation
• Review the new proxy lastError behaviour under real watchLastError emissions, cancellation, non-stale RPC failures
• Assess RunConfigDataProviderProxyTest coverage

Verdict: the server-side security property still holds and is now well covered - all eight bridge tests earn their keep. The new client-side change has two real defects, both small and both in the area you asked me to look at. Finding 1 makes the run panel's error banner permanently undismissable after one stale click; finding 2 tells the operator the wrong thing for every failure that is not a stale id, including a revoked token. I would fix those two before merge.

Base note: git merge-base origin/dev HEAD reports no merge base in this shallow checkout, so I reviewed this PR's own two commits (3f664c5, dac77a0) - +627/-38 over 4 files, matching the PR's own totals exactly. The sandbox refused ./gradlew, so I ran nothing; the new plugin-api-ipc test does land on the gated path (desktopTest runs under ./gradlew build, which is what AGENTS.md's "a bare ./gradlew test does not cover them" note is about).

---

Retained protections - re-verified at this head

All six RPCs gated. run_config.proto:15-30 declares six; all six are overridden and all six refuse without an identity. Nothing falls through.

Stream capture and per-emission revocation are correct, and I chased the interceptor this time. CURRENT_IDENTITY.get() is read in the RPC method body before the flow { } is collected; the closure holds the supplier, and ProcessIdentityInterceptor.kt:39 makes that supplier { registry.identityFor(token) } - re-resolved against the registry on every call, so a revoked token answers null, null != caller, and the stream refuses before emit. No value escapes after revocation. refuseIdentity returning Nothing is what smart-casts currentIdentity non-null inside the loop, so unlike DownloadServiceBridge.kt:47 there is no defensive re-test of an unreachable branch.

One property worth writing down: the mid-stream comparison is on the process id, not the token. If the registry ever re-issued a credential to the same process id, a live stream would continue on the new one. That is right for that case (same principal) but it makes this a principal check rather than a token check.

The execute allowlist is complete. Only the id is read; the provider's own copy is executed; command, arguments, working directory and environment are discarded. requestedId.takeIf { it.isNotBlank() } also closes the blank-id hole from my last review from the other side - with a non-blank requested id, a provider entry with a blank id is unmatchable, so the both-sides guard is satisfied in effect.

Test coverage server-side is now complete for everything the last handoff asked for: anonymous refusal of all six, authenticated happy path with forwarded scan path and window, the tamper regression, unknown id, unary revocation, mid-stream revocation, blank ids, and refusal during and after a rescan.

---

Findings

1. [correctness] The client-written lastError cannot be cleared - clearError() is a no-op against it, so one stale click leaves a permanent banner.

I traced the whole chain and it closes:

• The host's lastError is RunConfigurationManager.lastError verbatim (RunConfigurationDataProviderImpl.kt:33), written only by scanProject (DesktopRunConfigurationManager.kt:130, :143) and clearError (:153). execute is fire-and-forget through RunEventBus.execute (RunConfigurationDataProviderImpl.kt:51-58) and never writes it. So in the common case the host value is null.
• proxy.clearError() reaches DesktopRunConfigurationManager.clearError, which sets _lastError.value = null. MutableStateFlow conflates equal values, so null over null emits nothing.
• watchLastError therefore does not emit, and the proxy's locally written string survives.
• The next successful execute does not clear it either - RunConfigDataProviderProxyTest.kt:93 pins exactly that ("a successful execute does not touch lastError").

Net effect: after one stale Run click the run panel shows "This run configuration is no longer known - rescan the project or pick it again." forever, across every subsequent successful run, with no way to dismiss it. It clears only if the host's own lastError goes non-null and back to null (a scan fails, then another scan starts), or if the watch stream happens to reconnect. Before this commit a stale click reported nothing, once; now it can report wrongly, indefinitely - which is the worse of the two failures.

The fix follows from what the commit changed: the proxy now owns a client-origin value in this field, so it has to own clearing it. clearError() should set _lastError.value = null locally regardless of the RPC outcome, and a successful execute should clear a previously written client error - mirroring the host's own scanProject, which clears lastError before it starts.
Fix this →

2. [correctness] Every non-stale failure is reported as a stale configuration, including a revoked token.

catch (_: Exception) at RunConfigDataProviderProxy.kt:131 writes the same stale-id message for every outcome: UNAVAILABLE (kernel gone, channel shut down), DEADLINE_EXCEEDED, a transport CANCELLED, UNKNOWN (the host's execute threw), and - most misleadingly - PERMISSION_DENIED carrying NO_IDENTITY, i.e. a missing or revoked token. In that last case the configuration is perfectly fine, the caller is simply not authenticated, and "rescan the project" is advice whose ScanProject will also be refused. Given that this PR's own test suite adds two revocation tests on the server side, the client turning a revocation into "your configuration is gone" is the kind of mismatch worth closing in the same change.

The information is already on the wire: the bridge distinguishes the two refusals by description (NOT_A_KNOWN_CONFIGURATION vs NO_IDENTITY) and gRPC carries the code. Switching on (e as? StatusException)?.status?.code - PERMISSION_DENIED keeps the refusal wording, anything else gets "Could not start this run configuration (<code>)" - keeps every failure visible, which is the PR's whole point, and stops three of them lying. Coupling on Status.Code rather than on the description literal keeps the two modules from sharing a string.
Fix this →

3. [correctness - cancellation] Two of the three swallow sites still eat CancellationException, and the transport does not always use it.

scanProject (:113) and clearError (:148) keep a bare catch (_: Exception). Swallowing cancellation inside a suspend function lets a caller in a cancelled scope continue as though the call succeeded - which is precisely the bug this PR fixed in execute two lines up, so leaving two of three is drift with a fix already written next to it. clearError matters more once it becomes the local-clear path in finding 1.

Separately, cancellation does not always arrive as CancellationException: a call cancelled at the transport (channel shutdownNow, a server-side cancel) surfaces as StatusException(CANCELLED) and a deadline as DEADLINE_EXCEEDED, both of which land in the generic branch and write the stale message today. Finding 2's switch closes that half, so the two are worth doing together.

4. [doc accuracy] The new comment understates the overwrite and points at the wrong risk.

:132-140 says the message "can still be overwritten by the next watchLastError emission if the host's own lastError happens to change at the same moment." No change is needed for that: every reconnect of the watch loop re-collects the stream, and collecting a StateFlow replays its current value, so a reconnect after any stream fault (1s to 30s backoff) re-emits the unchanged host value and wipes the client message. And the direction that actually bites in practice is the opposite one - it sticks (finding 1) rather than being overwritten. The comment is also the right place to record that the host's lastError is a scan-error channel and this write widens it to carry a run refusal, because that widening is exactly what makes the clearing contract the proxy's problem rather than the host's.

5. [coverage] The proxy test's fake never serves watchLastError, so the entire interaction you asked about is invisible to it.

neverEmits() (:130, :137) means the host error stream emits nothing at all for the life of the test. That is a reasonable way to park the watcher coroutines, but it also means the suite cannot see the sticky clear (finding 1), the reconnect wipe (finding 4), or the ""-to-null round trip. Backing watchLastError with a MutableStateFlow<String> in the fake is a few lines and turns four behaviours from unobservable into asserted: refused execute writes; clearError() clears (fails today); a host emission of "boom" then "" tracks through; and a refused execute while the host error is non-null resolves deterministically one way.
Fix this →

6. [coverage] Cancellation and the non-stale failure classes are untested.

Deleting the catch (e: CancellationException) throw e at :129-130 leaves both new tests green, and so does changing the message to something wrong for an UNAVAILABLE. Two cheap additions: have the fake throw Status.UNAVAILABLE and assert the message does not claim the configuration is unknown (after finding 2); and cancel the calling coroutine while execute is in flight and assert CancellationException propagates rather than being converted into a lastError write.

7. [coverage - server side] watchLastError's value mapping is still untested in both directions.

RunConfigServiceBridge.kt:92 maps error ?: "", and the proxy maps back with takeIf { it.isNotEmpty() }, so a host error of literal "" is indistinguishable from null at the client. Harmless today, but this PR is the one that made lastError load-bearing for run feedback, so it is the right moment to pin it. The bridge suite has a success assertion for watchDetectedConfigurations only - watchIsScanning and watchLastError have refusal tests and no value tests.

---

Non-blocking notes

• refuseIdentity is reused for the mid-stream case, where the cause is identity changed or revoked, not absent - so a revoked stream logs "no current verified process identity on this call" for a call that did present one. A second constant, or a reason field in the map, would make a revocation legible in the host log.
• The refusal log phrasing is now split 5/3 across the family. Secret, Role, Supabase, Download and PluginUI log "no verified process identity on this call"; Git, Log and this bridge log "no current verified…". Not this PR's to resolve - the newer three agree with each other - but somebody should pick one so an operator can grep a single phrase across all eight.
• provider.calls.isEmpty() at RunConfigServiceBridgeTest.kt:109 is still vacuous for the three watch RPCs (the fake cannot record a property read). The assertFailsWith on take(1) is what has teeth there.
• The shared-module KDocs still name three gated bridges. ProcessIdentityInterceptor.kt:21-24 says "the three that currently make it"; the real count is eight now (Download, Git, Log, PluginUI, RoleManagement, RunConfig, Secret, Supabase). Pre-existing drift, and fixing it edits modules/boss-ipc, which widens this PR's blast radius - but it is the stale doc most likely to mislead whoever wires the next kernel server.
• scanProject's path and both RPCs' windowId remain unconfined (pre-existing, unchanged here). Remote UI: StreamUI has no plugin identity check — one plugin can bind another's surface #53 follow-up territory.
• The bridge's class KDoc paragraph re-wrapped unevenly at RunConfigServiceBridge.kt:37 (one 119-column line among 100-column neighbours). Under the 140 limit, so cosmetic.
• No UI was exercised, and the run panel that renders lastError is out of repo, so findings 1 and 2 are reasoned from the interface contract and the host's own writers rather than observed on screen. They are both mechanical enough that I am confident in the chain, but an actual stale click followed by a dismiss is the check worth doing by hand.
  · branch fix/run-config-caller-identity

[kshivang]

@Antriksh1984 Your original six identity gates/provider-held execution design and your new proxy error/cancellation work are useful and retained. I rechecked your update rather than repeating the old stale-Run handoff. All ten checks on your dac77a00 head pass; actual Linux/macOS/Windows XML confirms 8 bridge + 2 proxy tests per platform.

Current Claude review confirmed the security boundary and identified two concrete client defects: a locally written error could not be dismissed when host null-to-null StateFlow conflated, and every RPC failure was described as an unknown configuration. Reviewer repair e7ac153c0 is pushed to your original branch, without rewriting your commits. It separates local execution and host scan errors under one lock, preserves local failures across empty/replayed host snapshots, clears them on dismissal or successful retry, preserves host errors, and uses generic text without remote payloads or a false stale-id claim. It also propagates cancellation from Scan/Clear, as Claude requested. Seven additional proxy regressions cover these behaviors and in-flight cancellation. These are agent repairs, excluded from your preserved original 8.5/10 score.

Claude reconciliation: findings 1-6 addressed by the error-state repair, removal of the inaccurate overwrite comment, an emitting fake stream and new regressions. Optional additional bridge watch-value/property-access assertions remain test-strengthening, not a missing identity guard. Its suggestion to infer stale ids from PERMISSION_DENIED is rejected: that same code also means missing identity; generic wording is accurate for both. Its same-principal token-reissue observation is incorrect for the actual registry: CURRENT_IDENTITY captures the original token; issue invalidates the previous token, so the old stream resolves null, not the new token. Shared documentation/helper/runtime retry work stays #575; current dev has #651's integration fixes and that PR is confirmed merged.

Remaining gate is validation on the repaired head, not a demonstrated author/source defect. Diff whitespace/newlines pass. My first local command used an incorrect Gradle project path (reviewer error); the corrected command waited on the shared lock without starting Gradle and I cancelled only that queued command to keep this bounded. No local pass is claimed for the seven new tests, and prior-head green CI is not evidence for them.

Next steps: run python3 /Users/kshivang/Development/Boss/.pr-review-sessions/with-build-lock.py ./gradlew :plugin-platform:plugin-api-ipc:desktopTest --tests 'ai.rever.boss.plugin.ipc.RunConfigDataProviderProxyTest' :plugin-platform:plugin-api-ipc:ktlintCheck :plugin-platform:plugin-api-ipc:detekt; inspect new-head platform CI, then refresh dev/mergeability before restoring ready and merging into dev. The host bridge is unchanged from the validated eight-test version. No UI layout change, native visual exercise, external runtime publication or live database operation was performed. #653's Windows run disambiguation is distinct read-only context.

Converting to draft for this validation-only handoff under the requested bounded disposition policy.

[kshivang]

@Antriksh1984 Your six verified-identity gates, provider-held exact-id execution, and client-visible failure work are retained. I verified your repaired replay and its ten green checks rather than repeating the prior handoff. Original contribution rating remains 8.5/10; the additional repairs below are agent work.

Current dev/main were identical at 5a2e818. Your older ancestry conflicted with the promoted baseline in two documentation sections. Integration commit 940cf69 preserves current dev in both and leaves only the four intended RunConfig files in the net diff; your original branch history is retained by ordinary fast-forward pushes.

The single new Claude review confirmed the security boundary and found a valid follow-up: old local run feedback could mask a later failed scan. Repair 8cd4fe1 now lets a changed nonempty host failure supersede local feedback, and beginning a scan clears local run feedback. Unlike the suggested unconditional clearing on every nonempty emission, it does not treat a replay of the same host snapshot as necessarily newer. Generic text is now only “Run could not be started.” Cancellation tests observe that execution never continues after the cancelled Scan/Clear call (and join it); transport/refusal messages are compared directly; three new tests cover newer scan failure, successful run preserving host error, and scan clearing local feedback. Added caller-only scan audit, without logging caller-chosen paths.

Claude findings 1–6 are addressed by these changes. Finding 7 is already resolved in current dev: the interceptor no longer maintains a stale enumerated bridge list. Additional stream value/property-access tests and distinct revocation log stages remain optional coverage/diagnostic improvements; no missing guard is asserted. No repeated review request, and no independent Claude approval of the final repair is claimed.

Final local validation on repaired content PASS: 8 RunConfigServiceBridgeTest + 12 RunConfigDataProviderProxyTest, zero failures/errors/skips; composeApp and plugin-api-ipc ktlintCheck and detekt all pass, using the required with-build-lock.py. Intermediate failures were my test formatting, a mistyped test selector, and a test returning a non-Unit value; all corrected, not author defects. Logs and XML are retained in this sweep's artifacts.

Remaining gate is fresh hosted CI on 8cd4fe1, not a demonstrated remaining source defect. Converting to draft for a bounded validation-only handoff. Next steps: inspect the final head’s platform/build-summary, quality, compatibility and database checks; when green, refresh dev and verify the net diff/mergeability, restore ready and perform the authorized exact-head squash into dev under with-dev-merge-lock.py. If dev advances into the affected code/build path, revalidate the integration first. Do not substitute earlier-head green checks.

Contributor credit remains explicit in the PR description. #653 Windows run disambiguation and #575 shared transport/helper work are distinct read-only context, not duplicates. No native visual exercise, external runtime publication, app launch/stop or live database deployment. Existing plugin error-banner/dismiss wiring was source-inspected; no layout changed.

[Antriksh1984]

Confirmed current head `8cd4fe14` (scan-error precedence by recency + strengthened proxy regressions) - all ten hosted checks green, and locally: `RunConfigServiceBridgeTest` + `RunConfigDataProviderProxyTest` pass, ktlint/detekt clean on both `composeApp` and `plugin-api-ipc`. Restoring ready-for-review.

[kshivang]

@Antriksh1984 Rechecked your ready-for-review update against exact head 8cd4fe146b1722eeb9683b15470141fefc08c648. The prior validation-only blocker is resolved: all ten hosted checks pass in run 34838505127, with downloaded Linux XML confirming 8 bridge and 12 proxy tests, zero failures/errors/skips. Previous locked local tests and both modules’ lint/detekt also passed on these same bytes; no redundant rebuild is needed.

The latest Claude review (5663110604) remains applicable and its findings 1–6 are repaired: changed host error precedence, scan-start dismissal, neutral failure text, non-continuation assertions for cancellation, direct transport/refusal comparison, host-error retention and caller-only scan audit. Finding 7’s stale inventory is already absent in current dev. A repeated nonempty host snapshot is not necessarily newer, so the repair correctly avoids clearing newer local feedback merely on replay. Optional stream-value/property-read tests and distinct revocation diagnostics are not missing authorization gates. No unresolved review threads or demonstrated substantive defect remain; no new Claude request is necessary for this already reviewed/repaired head.

Six authenticated RPCs and exact nonblank provider-held execution satisfy the useful host security intent. Plugin error-banner/dismiss wiring remains compatible. Current dev’s intervening #650 changes are browser swipe navigation, not the bridge/adapter/build paths. #653 Windows disambiguation, #575 transport work and #666/#672 other identity bridges remain distinct read-only context; no duplicate is being closed.

Proceeding with the authorized exact-head squash into dev under the shared merge lock, subject to fresh state/check/mergeability preflight. Your original contribution and later fixes remain explicitly credited; original score stays 8.5/10, excluding prior agent repairs. No native visual or external-runtime end-to-end exercise is claimed; no layout changed.