Skip to content

fix(desktop): preserve authoritative connectionId, fail-closed legacy fallbacks — #90048 - #90198

Merged
teknium1 merged 2 commits into
NousResearch:mainfrom
andrexibiza:fix/89719-authoritative-connection-id
Aug 23, 2026
Merged

fix(desktop): preserve authoritative connectionId, fail-closed legacy fallbacks — #90048#90198
teknium1 merged 2 commits into
NousResearch:mainfrom
andrexibiza:fix/89719-authoritative-connection-id

Conversation

@andrexibiza

@andrexibiza andrexibiza commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Merged

This PR landed in main as 38ce2d7553c42af591a59087cb62ea018f9e6917 on 2026-08-23 and closed #90048.

Final authored head: 032078e843e556584dc0ef7a2845a9a1bfe5a68e.

Closed contract

Desktop connection identity remains registry-qualified from selection through activation:

  • Presence of a connectionId property is authoritative qualification.
  • Only a nonblank exact ID still present in the current registry is accepted.
  • Blank, malformed, unknown, and retired explicit IDs fail closed without endpoint fallback.
  • Only genuinely unqualified descriptors enter legacy compatibility matching.
  • URL and Cloud matching includes kind, normalized URL, auth mode, token envelope, canonical headers, and organization.
  • SSH matching includes normalized host and user plus defaulted port, key path, remote Hermes path, and remote profile.
  • Zero, multiple, weakly ambiguous, and exact-twin matches remain unresolved.
  • Registry ordering cannot change the verdict.
  • Remote-primary restoration carries the exact (connectionId, profile) tuple into gateway activation.

Implementation

  • apps/desktop/electron/connection-route-identity.ts owns the shared full-envelope matcher derived from fix(desktop): remote-primary desktops no longer duplicate every bot row — boot descriptor carries the connection id (salvage #88697) #88922.
  • apps/desktop/electron/desktop-remote-route.ts uses that matcher for pre-dial route selection.
  • apps/desktop/electron/connection-registry.ts owns property-presence/current-registry authority and ambiguity-safe legacy inference.
  • apps/desktop/electron/connection-registry.test.ts covers explicit-ID validity, local cardinality, URL/Cloud/SSH collisions, mixed-case SSH normalization, exact twins, and order invariance.
  • apps/desktop/src/store/connections.test.ts proves remote-primary restoration activates the exact registry ID.

No main.ts or profile-delete-routing.ts primary-as-owner heuristic is included. The independent stopgap from @AndreasG78 remains credited production evidence, not the canonical ownership rule.

Exact verification

The final PR head passed:

At final reconciliation, the merged commit is an ancestor of current main bdf10471b5dff0d67d172065f7dbf9f4c47c4c1b. Current-main source still contains the explicit current-registry fail-closed rule and the shared unique full-envelope matcher.

Provenance

Disposition: merged and complete. No implementation, verification, review, merge, or tracker work remains on this PR.

@andrexibiza
andrexibiza requested a review from a team August 19, 2026 18:33
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) labels Aug 19, 2026
@andrexibiza
andrexibiza force-pushed the fix/89719-authoritative-connection-id branch from 914d35d to 32aa366 Compare August 19, 2026 18:49
@andrexibiza

Copy link
Copy Markdown
Contributor Author

Implementation applied — exact-route-identity v2 (closes #90048 / #89719 class-closure)

Packet: `PR90048_EXACT_ROUTE_IDENTITY_V2_BUNDLE.zip` applied to a real worktree at base `bdc5b1f74c` (branch `fix/89719-authoritative-connection-id`). All 8 MANIFEST.sha256 hashes verified (a145f30201468b34a1e7e1e924208b72ca14508e0d423d97c258464f0b76d9a2).

What changed (source of truth: packet files/)

  • `apps/desktop/electron/connection-route-identity.ts` (new) — canonical identity sealing. Frozen pre-dial; identityForRoute, withConnectionId, matchingConnectionId in one seam.
  • `apps/desktop/electron/desktop-remote-route.ts` — import fix routing `matchingConnectionId`/`StoredRoute` through the new seam (removed stale `normalizeRemoteBaseUrl`/`normalizeRemoteHeaders`/`RegistryConnection` imports still referenced by the pre-fix(desktop): remote-primary desktops no longer duplicate every bot row — boot descriptor carries the connection id (salvage #88697) #88922 file).
  • `apps/desktop/electron/connection-registry.ts` — `matchingConnectionId` + `identityForRoute` impl (canonical URL/SSH normalization, duplicate-connection rejection).
  • `apps/desktop/electron/connection-registry.test.ts` — 228 lines ported for vitest.

Note on the patch: the patch's `@@ -1` hunk for `desktop-remote-route.ts` didn't apply verbatim because the file at base `bdc5b1f74c` had a JSDoc header shifting context and had been partially migrated (#88922 left `matchingConnectionId`/`StoredRoute` refs without imports). Resolved by applying only the import fix; the matcher-block removal hunk's target was already absent. Packet's `connection-route-identity.ts` applied verbatim.

Receipts (real execution)

  • tsc (/project tsconfig.electron.json): 0 errors (EXIT=0)
  • Vitest (--project electron connection-registry): 65 passed / 65
  • Harness (exact-route-identity assertions): `all assertions passed` (EXIT=0)
  • git diff --check: clean

CI / push (honest)

CI on the fork branch isn't auto-firing (same as #86432). Push to `axl-fork:fix/89719-authoritative-connection-id` needs --force-with-lease consent (branch ref exists from worktree checkout). Branch tip is at the verified head; awaiting explicit push or I'll raise a fresh PR from this state.

Class-closure

Closes the #89719/#90048 residual: alias-based route identity never sealed a canonical key pre-dial, allowing post-dial transport-metadata reconstruction to produce a stronger identity. v2 rebases identity on registry connection ID (frozen pre-dial) + canonical URL/SSH normalization, with duplicate-rejection. Addresses both #89461 findings (child-interposer bypass + transport-metadata reconstruction).

@andrexibiza

Copy link
Copy Markdown
Contributor Author

Verification update (post-rebase to current main b2057c168, force-push 95659ed2):

  • git diff --check clean.
  • Patch applies cleanly to current upstream/main via --reject --unidiff-zero (packet base_sha bdc5b1f7 is stale; rebased forward).
  • Extracted matcher verified byte-equivalent to fix(desktop): remote-primary desktops no longer duplicate every bot row — boot descriptor carries the connection id (salvage #88697) #88922's shipped matchingConnectionId.
  • TypeScript: 0 errors in target files against apps/desktop electron tsconfig (pre-existing connection-config.ts strict-mode debt excluded).
  • Focused harness: all assertions pass.
  • Vitest connection-registry.test.ts: baseline 62 → packet 65 tests, all pass (the prior backendScopeKey failure is pre-existing — identical on clean main without the patch; environmental @hermes/shared resolution).
  • **GitHub Actions: 19 success, 0 fail (incl. apps/desktop / check:lint green after fixing perfectionist import ordering in desktop-remote-route.ts); osv-scanner neutral (pre-existing).`
  • Mergeable state: clean. Open for review.

Copy link
Copy Markdown
Contributor Author

Topology interlock with #90913: retain this matcher work, but align the explicit-ID authority rule before landing

This PR is the stronger implementation/provenance source for full-fidelity legacy route matching. The shared connection-route-identity.ts contract correctly avoids URL/host first-match reconstruction by comparing the dimensions that distinguish legal registrations: remote/Cloud kind, auth mode, token/header/org envelope, and SSH port/key/path/profile. That work should survive consolidation.

#90913, however, has the stronger current explicit-identity rule: if a descriptor already carries connectionId, blank/null/undefined/unknown/retired IDs fail closed instead of falling back or continuing as current registered authority. That matches #90142's monotonic-qualified-identity invariant and #90149's distinction between a stable registration slot and the live generation currently authorized to use it.

So these PRs should not be treated as clean duplicates and should not both land independently. The composition target is:

  1. retain fix(desktop): make explicit connection identity authoritative #90913's explicit-ID/current-registry fail-closed semantics;
  2. retain this PR's shared full-fidelity matcher for genuinely property-absent legacy descriptors;
  3. retain this PR's duplicate-URL/auth/org/header and SSH port/key/path/profile witnesses;
  4. preserve this PR's contributor provenance on the surviving integration head;
  5. then close/supersede the non-surviving delivery surface with a literal backlink.

Downstream #90924 should consume that one canonical identity authority. It should not implement another compatibility matcher in the proof-carrying mutation layer.

Copy link
Copy Markdown
Contributor Author

Interlock with #90913 — overlapping owner, one semantic conflict

I compared the current patches and exact heads rather than treating these as independent follow-ups.

#90198 is the broader #89719 residual owner: it carries the shared #88922-style route matcher, full URL/Cloud auth/header/org envelope matching, SSH port/key/path/profile discrimination, weak-legacy uniqueness gates, and the desktop-remote-route.ts deduplication. Its exact head 95659ed2fae7e7592e76bfa6ba667cb5714fb3a6 is green in CI 32290263713, Docker 32290262526, and Nix 32290262399.

#90913 overlaps resolvedConnectionId() and adds one stricter rule not equivalent to this branch: when a connectionId property is present but blank/null/unknown, it fails closed; for a non-empty ID it additionally requires current registry membership. This branch deliberately preserves a non-empty descriptor ID even when the registry entry has retired, keeping identity proof separate from current registry liveness.

That is a real architecture decision, not a duplicate-test detail. Do not merge both independently. #90198 should remain the full-envelope/topology owner; #90913 is an interlocked explicit-ID/liveness-policy refinement whose chosen rule must be composed here (or explicitly rejected) before either path lands. Refs #90913.

Copy link
Copy Markdown
Contributor Author

Canonical composition decision: #90198 is the intended surviving delivery surface, but it is not merge-authorized until it absorbs #90913's explicit-ID liveness rule.

Exact evidence revalidated now:

The semantic union is precise:

  1. Keep fix(desktop): preserve authoritative connectionId, fail-closed legacy fallbacks — #90048 #90198's shared matchingConnectionId full-envelope legacy matcher for URL/Cloud auth-mode/token/header/org identity and SSH host/user/port/key/path/profile identity, including ambiguity rejection.
  2. Replace fix(desktop): preserve authoritative connectionId, fail-closed legacy fallbacks — #90048 #90198's current non-empty-string fast path with fix(desktop): make explicit connection identity authoritative #90913's property-presence rule: if connectionId is present, blank/null/undefined/non-string/unknown/retired IDs return null; only an exact ID present in the current registry is accepted. No explicit-ID failure may fall back to endpoint inference.
  3. Retain the collision/regression matrix and add the fix(desktop): make explicit connection identity authoritative #90913 explicit blank/null/undefined/unknown/current-ID witnesses on the same surviving head.

I am deliberately not closing #90913 yet: its liveness invariant is still absent from #90198's production code. Retirement condition is the exact composed #90198 object + green hosted receipts, after which #90913 can self-close as superseded with its provenance preserved.

Copy link
Copy Markdown
Contributor Author

Current consolidation decision after re-reading both exact heads: this PR remains the intended surviving delivery authority, but is not merge-authorized in its present state.

Required current-main composition:

  • retain this branch’s shared connection-route-identity.ts full-envelope URL/Cloud/SSH matcher, collision witnesses, and reuse from both pre-dial and registry compatibility paths;
  • replace its current if (String(connectionId).trim()) return connectionId rule with fix(desktop): make explicit connection identity authoritative #90913’s property-presence/current-registry rule: once the descriptor contains connectionId, blank/null/undefined/unknown/retired IDs return null and never become legacy inference candidates;
  • change the existing retired-ID regression from preservation to fail-closed rejection and absorb fix(desktop): make explicit connection identity authoritative #90913’s focused invalid-explicit-ID witnesses and provenance;
  • rebase the single survivor on current main, then require fresh exact-head CI/Docker/Nix.

GitHub now reports both #90198 and #90913 nonmergeable. No third PR will be opened, neither old head’s green will be inherited, and #90913 must remain open until this branch actually contains its load-bearing semantics and tests.

andrexibiza added a commit to andrexibiza/hermes-agent that referenced this pull request Aug 21, 2026
Keep explicit connection identity authoritative only while its registry entry is live, and route every legacy compatibility inference through the same complete pre-dial auth and transport envelope used by Desktop route selection.

Absorbs the full matcher and collision matrix from NousResearch#90198 into the stricter current-registration contract in NousResearch#90913.
Make explicit registry qualification authoritative: only a current exact ID is accepted, while blank, malformed, unknown, or retired claims fail closed without endpoint inference. Restrict genuinely unqualified legacy descriptors to the shared full-envelope URL/Cloud/SSH matcher, reject zero or multiple matches, normalize SSH host/user identity, and prove remote-primary restoration keeps the exact (connectionId, profile) tuple.

Closes NousResearch#90048.

Prior work by @teknium1 in NousResearch#89719 and NousResearch#88922, @andrexibiza in NousResearch#90913, and @AndreasG78 in NousResearch#90048 (comment) shaped this implementation. @saralilyb's NousResearch#90006 remains downstream consumer context; the production stopgap is credited but excluded because registry primary does not prove route ownership.

Copy link
Copy Markdown
Contributor Author

Published the composed current-main repair at exact head 9773a3dd37530024df2d915d2e1e2339ecc6153d (parent 2ea5287da0ed3556110e5e5c88210b41db19ec6d; exact tree c04e6a3295dd111867f8f4505792d87ace7f0624).

What changed on the survivor:

Local exact-head gates:

  • focused Electron: 78/78;
  • adjacent UI caller/lifecycle: 35/35;
  • Electron + renderer TypeScript: pass;
  • targeted ESLint, diff check, attribution audit, Windows-footgun scan: pass.

The broader sweep's only failures are outside the changed paths: this sandbox cannot run one ps-dependent Electron test because ps -p $$ itself fails, and existing model-menu UI tests remain timing/state-sensitive. Exact details are recorded in the PR body.

Prior work by @teknium1 in #89719/#88922, @andrexibiza in #90913, @AndreasG78 in #90048 (comment), and downstream context from @saralilyb in #90006 are explicitly preserved. Fresh hosted CI/Docker/Nix now own the retirement gate.

Copy link
Copy Markdown
Contributor Author

Exact-head acceptance is complete for 9773a3dd37530024df2d915d2e1e2339ecc6153d:

#90913's invariant/evidence has been absorbed and that PR is now closed unmerged as superseded. #90006 and #90924 remain downstream consumers to rebase after this identity owner; #89769 remains disjoint roster work with a whichever-lands-later rebase.

The canonical boundary is unchanged by the production stopgap: exact current ID when qualified; full-envelope, ambiguity-safe matching only when connectionId is truly absent. No registry-primary-as-owner heuristic was introduced.

Prior work by @teknium1 in #89719/#88922, @andrexibiza in #90913, @AndreasG78 in #90048 (comment), and downstream context from @saralilyb in #90006 remain credited. #90198 is ready for maintainer review/merge; #90048 should close through the existing Closes #90048 interlock when this PR merges.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 9773a3dd37530024df2d915d2e1e2339ecc6153d. I found no code blocker in the connection-identity contract.

This is the surviving resolver authority:

  • property presence makes an explicit connectionId current-registry-or-fail-closed; malformed/retired claims never fall through to endpoint inference;
  • genuinely unqualified legacy descriptors reuse one canonical full-envelope matcher;
  • URL/cloud identity includes kind, normalized URL, auth mode, token/header envelopes, and org;
  • SSH identity includes host, user, port, key, Hermes path, and remote profile;
  • zero or duplicate matches remain unresolved and registry order never becomes authority.

Topology decision: keep #90913 closed. #90006 and #90924 must consume connection-route-identity.ts / resolvedConnectionId() rather than introducing a second matcher, endpoint-derived fallback, or receipt-local reconstruction. Stable ID and generation fencing can be layered on the resolved identity; they must not replace the resolver that proves which current registry object the request names.

Preserve the exact NousResearch#90198 implementation tree for its five owned paths while composing upstream main b6bcb3e as the second parent. The intervening main delta does not touch any NousResearch#90198-owned file; this is topology/current-main composition only, not a regenerated identity repair.

Copy link
Copy Markdown
Contributor Author

Exact-head receipt refresh: the live PR head is now 032078e843e556584dc0ef7a2845a9a1bfe5a68e, not the earlier 9773a3dd... object cited in prior closure comments. For this exact current head, CI 32544489752, Docker 32544489257, and Nix 32544489239 all completed successfully; review-thread census is empty. The older green remains historical evidence only.

@teknium1
teknium1 merged commit 38ce2d7 into NousResearch:main Aug 23, 2026
42 checks passed
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

The fail-closed redesign is coherent and unusually well-tested: presence-of-property semantics for connectionId claims (invalid claims stay unresolved instead of degrading to endpoint inference), ambiguity returning null everywhere (duplicate locals, twin registrations, reordered arrays), and the shared matcher extracted so legacy and registry paths can't drift. The order-independence assertions are the right paranoia.

One trust-boundary question worth documenting:

  1. Explicit claim wins without envelope equality: a descriptor stamped connectionId: 'remote-b' carrying remote-a's token resolves to remote-b (the test pins this as intended). That makes whoever mints the stamp load-bearing — if a renderer ever persists a descriptor across an auth edit, a stale claim silently adopts whichever source it names. Worth a comment (or invariant doc) stating where connectionId is attached and that it must never survive descriptor mutation; a debug log when a claim is accepted despite a mismatched envelope would make field misdiagnosis possible.
  2. Normalization tightening is a behavior delta: the shared routeIdentity now trims/lowercases SSH host and user (the old desktop-remote-route.ts copy compared raw values), so pre-existing registries holding case-differing duplicates that used to match distinctly now collide and fail closed via unique. Almost certainly intended, but it's invisible in the diff shape — a changelog/test noting the migration would help anyone debugging "my remote stopped resolving".
  3. Minor: normalizedSshTarget() in connection-registry.ts re-implements the coarse half of the ssh identity; routing both through routeIdentity (or extracting the host/user pair builder) leaves one normalizer.
  4. stableValue recursion is fine for JSON-derived config but will stack-overflow on cyclic input — fine today, worth an assertion if the descriptor surface ever widens beyond config literals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop: preserve exact registry connectionId; reject ambiguous legacy route inference

4 participants