fix(desktop): pin/unpin mirroring — pull-pass reverts brand-new pins and resurrects just-unpinned ones

[Hinnamnor17]

Symptom: Sidebar session pin/unpin has no visible effect. Pinning a session removes the pin in the same tick it is created (and wipes stored pins on boot); unpinning a session resurrects it instantly — in both cases the PATCH never reaches the backend.

Root cause: pullRemotePins() in apps/desktop/src/store/session-pin-sync.ts runs before the push pass on every reconcile and treats the server row as authoritative. The sessions list endpoint projects an explicit pinned: false on every unpinned row, so:

• Pin: a brand-new local pin (whose PATCH has not been sent yet) is read as "removed elsewhere" and immediately unpinned.
• Unpin: a mirrored pin the user just removed is read as "pinned elsewhere" (the server page still says true) and immediately re-pinned.

The existing unconfirmed guard only protects writes already in flight, not intents born in the same reconcile.

Fix: Gate both directions on the mirrored set (pins this app has already PATCHed true):

• Server false is only authoritative for a previously-mirrored pin — brand-new local pins survive to the push pass and get PATCHed.
• Server true is only authoritative for a pin we did NOT mirror — a mirrored pin's local absence means the user just unpinned; the unpin PATCH goes out.

     if (row.pinned && !heldLocally) {
+      // Server-true is only authoritative for a pin we did not mirror
+      // ourselves. For a mirrored pin, local absence means the user just
+      // unpinned — the page predates the unpin PATCH our push pass is about
+      // to send. Adopting the stale true would resurrect the pin in the
+      // very reconcile that removed it, and the unpin write never goes out.
+      if (mirrored.has(pinId) || mirrored.has(row.id)) {
+        continue
+      }
       pinSession(pinId)
       mirrored.add(pinId)
     } else if (!row.pinned && heldLocally) {
+      // Server-false is only authoritative for a pin the server previously
+      // acknowledged (tracked in `mirrored`). A brand-new local pin has not
+      // been PATCHed yet, so its server row still says false — reading
+      // "never heard of it" as "removed elsewhere" undoes the user's pin in
+      // the very reconcile that created it (and wipes stored pins at boot).
+      if (!mirrored.has(pinId) && !mirrored.has(row.id)) {
+        continue
+      }
       unpinSession(local.has(pinId) ? pinId : row.id)
 }

Tests: Added 3 regression tests in apps/desktop/src/store/session-pin-sync.test.ts (brand-new pin with explicit server false; boot re-assert of stored pins; local unpin while the server page still says true). Verified RED on unfixed code, GREEN with fix — 14/14 pass.

[teknium1]

Thanks for isolating the pull-before-push race; current main does perform the conflicting pull at apps/desktop/src/store/session-pin-sync.ts:84-102.

Problems

• The new mirrored gate conflates a local action made in this process with an old localStorage value. mirrored is empty after restart, so a stored pin paired with a server pinned:false row is preserved and then PATCHed back to true. That resurrects an unpin made by another Desktop app while this app was stopped. The current contract explicitly calls a present server pin row authoritative (apps/desktop/src/types/hermes.ts:490-495; introduced with cross-app sync in 8ce8b70dca).

Suggested changes

• Represent newly initiated local intent separately from restored localStorage state (or use a one-time migration marker), then add a restart-after-remote-unpin regression test alongside the immediate local-pin race tests.

Automated hermes-sweeper review.

[teknium1]

mirrored is empty after every restart, so this also preserves stale localStorage pins rather than only a newly initiated local pin. If another app unpinned this session while this app was stopped, its authoritative pinned:false row is skipped here and the later push pass PATCHes it back to true. Track current-process local intent separately from restored pins.

[alt-glitch]

This was generated by AI during triage.

Related competing repairs for the Desktop pin-sync race: #75295, #74788, #74638, #74444, and #74418. This patch uses bidirectional mirrored-state guards and also covers boot restoration; maintainer selection of one reconciliation mechanism is needed.

[Hinnamnor17]

Reworked per review — newly initiated local intent is now tracked separately from restored localStorage state.

What changed:

• Added a localIntent set populated only by $pinnedSessionIds changes that occur outside reconcile (user clicks in this process). Reconcile's own adopt/drop mutations are excluded via an inReconcile guard, and the boot-restored set is the listener baseline, so it never counts as intent.
• Server-false is now authoritative for any pin the server has heard about: a stored pin whose present row says false is dropped at boot (a remote unpin made while this app was stopped stays dead, per the cross-app contract at types/hermes.ts:490-495), while a pin clicked in this process survives the stale page until its PATCH lands.
• Server-true adoption keeps the mirrored gate: a mirrored pin's local absence is the user's own unpin, not a remote pin.

Tests (14/14, RED to GREEN):

• kept: brand-new pin with explicit server false; local unpin while the server page still says true
• replaced the old boot re-assert test with the requested restart-after-remote-unpin regression test: stored pin + present server row false -> dropped, no PATCH sent.

[GottZ]

This was generated by AI during triage.

Summary

Three open PRs address the same Desktop session pin-sync pull-before-push race: #74638 adds a just-changed guard, #75295 makes local pin-set reconciliation push-only, and #75342 separates current-process intent from restored localStorage while preserving server authority at boot. No PR has a recorded Verify verdict or best_fix status.

Related pull requests

• fix(desktop): prevent stale session page from undoing pin/unpin clicks #74638 duplicate — (+69/-2) — verify n/a: Adds a symmetric-difference justChanged guard so stale session rows cannot reverse a pin or unpin before its PATCH is issued. The automated keep_open review identifies an unaddressed rapid opposite-click race: older write completions can clear the newest unconfirmed guard, so this remains a narrower competing repair.
• fix(desktop): stop pin-sync pull from reverting the user's own pin click #75295 duplicate — (+44/-5) — verify n/a: Tags reconciliation sources and skips the remote pull for local pin-set changes, directly preventing stale pinned=false/true rows from undoing the user's click while retaining boot and session-refresh pulls. The automated keep_open verdict supports this focused salvage path; discussion also identifies it as a duplicate of fix(desktop): preserve local pin clicks during server sync #74788.
• fix(desktop): pin/unpin mirroring — pull-pass reverts brand-new pins and resurrects just-unpinned ones #75342 related — (+154/-33) — verify n/a: Adds bidirectional mirrored-state guards plus localIntent tracking and a restart-after-remote-unpin regression test, addressing the contributor's objection that restored localStorage pins must not be resurrected. Despite the keep_open review on fix(desktop): pin/unpin mirroring — pull-pass reverts brand-new pins and resurrects just-unpinned ones #75342, this is a larger competing implementation of the same race and its distinct boot-authority coverage can be ported to the consolidation PR.

Duplicates

#74638, #75295, and #75342 are substantially duplicate repairs of the same stale pull-before-push race. The discussion additionally identifies #75295 as a duplicate of #74788, so the chain is #74638/#75342 → #75295 → potentially #74788.

Suggested consolidation

Keep #75295 open with a salvage path, consistent with its automated keep_open verdict: add generation-safe per-ID handling and deferred pin→unpin/unpin→pin coverage for the overlapping-write gap, and port #75342's restart-after-remote-unpin regression. Then close #74638 as a duplicate of #75295 because its diff lacks that coverage despite the keep_open review on #74638, and close #75342 as a duplicate of #75295 despite the keep_open review on #75342 because the diff's unique boot test can be salvaged without retaining its duplicate synchronization design; resolve the existing #75295/#74788 duplicate relationship before closure.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup74638 ["PRs duplicating each other"]
        P74638["PR #74638 (open)"]
        P75295["PR #75295 (open)"]
        P75342["PR #75342 (open)"]
    end
    class P74638 open
    class P75295 open
    class P75342 open
    class P75342 target
    click P74638 "https://github.com/NousResearch/hermes-agent/pull/74638"
    click P75295 "https://github.com/NousResearch/hermes-agent/pull/75295"
    click P75342 "https://github.com/NousResearch/hermes-agent/pull/75342"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 19 kB of PR diffs, 7 kB of issue/PR text, 3 kB of discussion (6 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.