feat(desktop): carry route activation proof into file mutations - #90924
andrexibiza wants to merge 1 commit into
Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 4698d2e708bbedaa987e24797ae56743bd5aa737. The proof-carrying activation work itself is directionally strong: generation fencing, exact (connectionId, profile) no-op matching, descriptor provenance, gateway-realization revalidation, and the separation between degraded display/navigation and ready mutation authority are all the right shape. The focused contention/owner tests exercise the important N vs N+1 and same-profile/different-source cases.
I found one substantive architecture blocker and one concrete landing blocker.
Blocker: the Git half of #89916 still drops route authority
The PR summary says it “routes read-only Desktop FS/Git facade calls through the gateway-owned connection context,” and #90866 explicitly names filesystem and Git mutation as the proof-carrying boundary. The FS half is fixed here, but apps/desktop/src/lib/desktop-git.ts is not in this PR’s seven-file diff and still does this on current main:
return desktop.api<T>(
body ? { body, method: 'POST', path, profile: desktopFsProfile() }
: { path, profile: desktopFsProfile() }
)That means it still carries only the bare profile and bypasses the connection-aware hermesApi() owner for reads. More importantly, every remote Git POST continues to execute without the new mutation receipt: worktreeAdd, worktreeRemove, branchSwitch, review.stage, review.unstage, review.revert, review.commit, review.push, and review.createPr all flow through the same profile-only gitPost().
This is not a speculative adjacent improvement. #89916, reported by @frizikk, identifies both desktop-fs.ts and desktop-git.ts as the same wrong-owner defect class, and #90866 states the governing invariant directly: fail-open display/navigation must not become fail-open filesystem or Git mutation authority. Leaving Git unchanged gives us exactly the asymmetric failure this architecture is supposed to eliminate: file save is fenced by a current exact receipt while branch/worktree/index/commit/push mutations can still be sent to whichever backend a profile-only request resolves to.
Required closure: carry the same route proof through the Git facade. Read-only Git calls should preserve the active registry owner (at minimum via the connection-aware API path), and consequential Git POSTs should consume a current ReadyRouteActivationReceipt at the side-effect boundary and send the receipt’s exact connectionId + profile, failing closed when only inferred/unqualified proof exists. Please add focused witnesses for (1) same profile name on two registry sources, (2) exact-owner Git read routing, (3) mutation rejection without current exact proof, (4) exact owner/profile on a permitted mutation, and (5) local Git remaining unchanged.
This is also the “other side of the shape” here: a proof object is only useful if every mutation surface in the defect class consumes it. Otherwise the system has a strong fact at activation and selectively projects it back to a bare profile at another facade.
Landing blocker: exact-head CI is red, and the branch is now materially behind main
The exact-head CI run 32393551173 has all Desktop test shards green, but apps/desktop / check:lint fails on this PR’s profile.ts import ordering:
apps/desktop/src/store/profile.ts:19:3
perfectionist/sort-named-imports
Expected "activateGatewayAgentWithProof" to come before "activeGatewayRouteMatches"
Nix and Docker are green. Current main is now 5e32e3aecd2070e8245d9ae2c9ee257f547fcb71; this head still has merge-base f43eabee5f36e11448086ee8ee17c499958e81bf and is 15 commits behind. The intervening main files do not overlap this seven-file diff in the compare I checked, so this looks like a straightforward rebase rather than a semantic conflict, but the final receipts need to be rerun on the rebased head.
Interlocks / ownership
- #89719 and #89797 are the landed route-identity / atomic-activation substrate this PR is correctly extending, not work to duplicate.
- #90048 is the remaining weak-identity/inference side; #90913 is complementary Electron-side identity hardening, not a duplicate of this renderer mutation-proof layer. Revalidate this PR after that identity owner moves so
registryScoped/explicit-ID semantics remain aligned. - #89916 is the concrete FS+Git resource-routing defect. This PR currently closes only the FS half.
- #90866 remains the architecture umbrella; this should be its first complete Desktop source→observable→side-effect proof path only once the Git mutation surface is brought under the same authority contract.
Once the Git facade consumes the same proof boundary, the lint error is fixed, and the branch is rebased/reverified on current main, I think this becomes a strong first implementation of #90866 rather than a partial proof island.
4698d2e to
92d0cf0
Compare
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 92d0cf04fb8b16b71c891233ea9427d8b9094755 after the Git-surface completion and current-main rebase. The prior Git authority and import-order blockers are closed: all consequential remote Git methods now consume the current receipt, read-only Git uses the connection-aware path, and the focused same-profile/different-owner witnesses are present.
Two architecture blockers remain before this is the claimed complete source → observable → mutation proof path.
1. Degraded/failed activations can permanently strand mutation authority
activeGatewayRouteMatches() currently treats any current landed receipt—including status: 'degraded'—as satisfying the no-op gate, then falls back to the physical gateway route even when the latest receipt is rejected.
Both states are reachable in production:
- A cold activation can select the requested route while the socket is not yet open or the descriptor lookup is unavailable.
finishActivation()publishes a degraded receipt. A later reconnect/published descriptor does not upgrade or replace that receipt, butensureGatewayProfile()now returns early becauseactiveGatewayRouteMatches()says the route already matches.currentRouteActivationReceiptForMutation()therefore remains null indefinitely even after the exact route is healthy. - A failed activation advances
gatewayActivationEpoch()but can leave the previous physical route active. That invalidates the previous ready receipt and publishes a failed receipt. Selecting the still-active route then hits the physical-route fallback and skips the only operation that could mint a new ready receipt. FS/Git mutations remain fail-closed forever until an unrelated route switch or reload.
This is not merely conservative denial; it is an unrecoverable authority dead-end created by the proof layer itself.
Required closure:
- Separate “display route already selected” from “current mutation proof exists.” The mutation-authority no-op must require a current ready receipt that still passes
routeActivationReceiptAllowsMutation(...), or the activation path must explicitly refresh/upgrade degraded proof. - Do not let the physical-route fallback suppress reactivation after a rejected current-generation attempt.
- Add production-shaped witnesses for: closed-at-land → reconnect-open; descriptor-unavailable-at-land → later descriptor publication; failed B activation leaving A physically active → reselect A; and repeated same-route selection after each case. Each must recover a ready receipt without switching through an unrelated route or reloading.
2. The proof is dropped before the real cross-process side-effect boundary
remoteFsMutationApi() and desktopMutationApi() validate the renderer receipt, but then project it to only connectionId + profile in window.hermesDesktop.api(...). Electron main's handleHermesApiRequest() calls ensureRegistryBackend(connectionId, profile) against the current registry entry. No activation generation, registry revision, descriptor fingerprint, or main-minted capability crosses IPC.
Therefore a receipt for source id X/endpoint generation N can authorize a mutation after X has been edited or rotated to endpoint generation N+1. During the registry-change/broadcast window, the renderer can still be displaying and connected to N while main re-resolves the same stable id to N+1 and executes the Git/filesystem side effect there. Exact string identity is not exact physical-route identity across this boundary.
Required closure:
- Carry a main-authoritative source/descriptor generation (or an opaque main-minted mutation capability) through activation and into the IPC request.
- Validate that proof in main immediately before
ensureRegistryBackend/dispatch; stale, removed, or replaced source generations must fail closed rather than being reinterpreted under the same id. - Add a witness that edits/removes/recreates a registry source between renderer activation and FS/Git mutation and proves the stale receipt cannot reach the successor endpoint.
- Keep #90913 as the explicit-ID/inference interlock, but do not treat it as this generation proof; it makes ids authoritative, not immutable across source replacement.
Current exact-head Docker is green; CI is queued and Nix is still running as of this review. Even once those finish, they will not cover the two state-transition classes above without the missing witnesses.
Temporarily install exact-branch repair authority for the proof-carrying route PR.
Remove the temporary workflow after the exact branch repair landed.
The exact target branch is repaired and its branch-local runner is gone; remove the temporary default-branch authority.
|
CI follow-up for exact head Run All desktop test shards, desktop platform tests, plugin tests, typecheck, OSV, Docker, and Nix are green at this head. Reordering that one named import closes the mechanical CI failure; it does not change the two substantive proof/recovery blockers in review |
Temporary fork-only workflow to repair the NousResearch#90924 typecheck failure and replace the source branch history with one commit on current upstream main.
10c5af8 to
a4195c7
Compare
c8ba81a to
7970d39
Compare
|
Implemented and verified the closure on a single clean commit rebased directly onto current Exact head: The branch now contains one commit; the red intermediate history was removed. Exact-head verification is green: Final repairs included the source-generation proof boundary, exact generated authority for remote file/Git mutations, source replacement/recovery witnesses, the plugin-socket fixture, registry literal typing, and lint-clean import ordering. |
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review on exact current head 7970d3933a72235a192a9c3332ad73b5a7c6c101: the two blockers from my review at 4698d2e... are closed.
Git half of #89916: desktop-git.ts now separates read authority from mutation authority correctly. Remote reads flow through hermesApi(), preserving the active registry connectionId + profile; every consequential remote Git mutation (worktree add/remove, branch switch, stage/unstage/revert, commit, push, create-PR) consumes currentRouteActivationReceiptForMutation() and sends the receipt's exact connectionId, profile, and source routeGeneration. Missing/stale/unqualified proof throws before the REST mutation. desktop-git.test.ts now has the exact witnesses requested in the earlier review: same-profile/different-source reads, local unchanged behavior, ambient-route disagreement with receipt-owned mutation, all mutation surfaces rejected without proof, source-generation refusal, and branch→worktree ownership continuity.
Landing state: the PR is now based on current main ee000768cef4dc9399f32c88b507104ce15400dd, and the exact head is green on all three hosted workflow families: CI 32426094889, Docker 32426094471, and Nix 32426094398.
That changes the architecture verdict materially: this is no longer an FS-only proof island. It now carries route-generation authority from activation through both FS and Git mutation boundaries, while retaining degraded/read-only compatibility separately from mutation authority. #90866 remains the repository-wide umbrella, but this PR is now a coherent first complete Desktop source → observable publication → side-effect proof path for the #89916 class.
One topology constraint remains external to this PR: the Electron-side explicit-ID/legacy-resolution work in #90913 and #90198 must converge on one canonical identity contract. This branch should consume that contract after it lands; it should not absorb or duplicate that resolver work here.
Replace the completed repair runner with a path-scoped cleanup that deletes the temporary recovery branches.
Register the exact-branch pull-request trigger on default main so the recovery-ref deletion can execute under an observable workflow run.
Add a temporary server-generated schedule so the recovery-ref cleanup does not depend on connector-originated events.
Remove the temporary NousResearch#90924 recovery workflow after confirming connector and scheduled events did not execute branch deletion.
Exact-head hosted verification receiptVerified current head
This closes the hosted-verification residue for this proof-carrying Desktop route-activation slice. It does not expand the PR’s stated scope or claim closure of the repository-wide #90866 architecture umbrella. |
|
@teknium1 @helix4u — maintainer review requested on exact head GitHub's formal reviewer-request endpoint is unavailable to the connected integration, so this is the public handoff. There are no unresolved inline threads. Direct exact-head receipts are CI 32426094889, Docker 32426094471, and Nix 32426094398, all successful. The acceptance invariant is now one coherent class boundary: both remote FS and every consequential Git mutation consume the current exact route activation receipt—including source generation—while legacy/inferred routes remain usable for read/display compatibility but cannot acquire mutation authority. The only remaining topology dependency is external: #90913/#90198 must converge on one Electron-side identity resolver. This PR should consume that owner after it lands rather than duplicating resolver semantics. |
|
@teknium1 maintainer review requested on exact head I revalidated before this handoff: GitHub reports the PR mergeable/non-draft, and the exact head remains green on CI Formal reviewer-request mutation is unavailable to this integration ( |
|
Collision/interlock note: #90198 is the upstream identity-authority owner and is now published at exact head Merge order: land #90198 first after hosted acceptance, then rebase #90924 onto its exact-ID/current-registry contract. Keep identity resolution in #90198 and mutation authorization here. |
|
Composition requirement after review of #90198: #90198 is the surviving connection-identity resolver. This PR may own route-generation fencing and proof-carrying activation, but generation is version proof for an already-resolved registry object; it cannot establish which object the request names. Restack on #90198 and bind the receipt to its result:
Do not reconstruct identity inside |
This is a well-structured piece of security engineering: main-process-owned generation tokens minted on dial-material changes ( Points:
|
|
Topology changed materially under this route-activation work and the next composition should delete duplicated substrate, not preserve historical overlap. Two prerequisites are now shipped on current main:
That strengthens the premise of this PR, but it changes its job. #90924 should now be the proof-carrying activation→effect layer only: consume current registry identity and connection-qualified Bot routing as existing authorities, then carry/revalidate generation/provenance at consequential FS/Git mutation boundaries. It should not recreate connection resolution, Bot source routing, or legacy exact-ID fallback logic already in main. The active head
The other side of the shape to keep explicit is revocation: connection removal/dial-material replacement must invalidate the receipt before any later stable-ID reuse can become mutation authority. Presentation/read-only compatibility may degrade gracefully; mutation must remain exact and current. |
Summary
RouteActivationReceiptthat carries exact owner, activation generation, gateway realization, descriptor provenance, epistemic status, and readiness across Desktop activationRoot cause
The gateway registry already maintains an activation epoch and qualified
(connectionId, profile)route, butprofile.tsprojected activation tovoid | booleanplus a separately resolved descriptor. Downstream code reconstructed authority from a bare profile,$connection, or endpoint-shaped state.desktop-fs.tsthen droppedconnectionIdentirely before the side effect.That allowed:
Guarantees
connectionId + profileto the bridgeTests
Local validation:
git diff --checkScope
This is the first complete Desktop source → observable publication → mutation-boundary proof path for #90866. It does not close the repository-wide architecture umbrella; updater, persistence, transport, and packaged-realization paths remain separate migrations.
Refs #90866
Refs #89916
Refs #90048