Skip to content

fix(api_server): persist and round-trip session pinned/archived via PATCH - #76007

Closed
SongotenU wants to merge 1 commit into
NousResearch:mainfrom
SongotenU:fix/api-session-pin-archive
Closed

fix(api_server): persist and round-trip session pinned/archived via PATCH#76007
SongotenU wants to merge 1 commit into
NousResearch:mainfrom
SongotenU:fix/api-session-pin-archive

Conversation

@SongotenU

Copy link
Copy Markdown
Contributor

Summary

The desktop pin/unpin + archive surfaces PATCH /api/sessions/{id} with {"pinned"} and {"archived"} (apps/desktop/src/store/session-pin-sync.ts, use-session-actions/index.ts:1429, sessions-settings.tsx:74), but the gateway rejected both fields with 400 unsupported_session_field and never returned them in the session payload — so the client's pull/reconcile passes saw stale state and the "rows now carry pinned" pull path was effectively dead code.

This fixes the whole bug class, not just one call site:

  • _handle_patch_session — accept pinned/archived in the PATCH allow-list and persist them via set_session_pinned / set_session_archived (using "pinned" in body, not truthiness, so unpin/unarchive with false actually reaches the DB).
  • _session_response — include pinned/archived in the client-safe keys so the response round-trips the new state.
  • _handle_list_sessions — pass include_pinned=True so pinned sessions survive pagination instead of silently dropping out of the sidebar (the backfill logic already exists in list_sessions_rich).
  • session-pin-sync.ts — guard the pull pass against re-pinning an id that was just unpinned, and surface unpin failures instead of swallowing them.

Tests

  • New TestSessionPatchEndpoint in tests/gateway/test_api_server.py: pin/archive persistence, unknown-field rejection, and the false (unpin/unarchive) toggle path.
  • scripts/run_tests.sh tests/gateway/test_api_server.py tests/test_hermes_state.py — 235 passed, 0 failed.
  • npx vitest run src/store/session-pin-sync.test.ts — 11 passed.
  • npx tsc --noEmit — clean.

@SongotenU
SongotenU force-pushed the fix/api-session-pin-archive branch from 7b912e9 to 593bae4 Compare August 1, 2026 08:03

@pestoura pestoura left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new PATCH fields need strict boolean validation before persistence. bool(body["pinned"]) and bool(body["archived"]) silently reinterpret valid JSON values of the wrong type: for example, {"pinned": "false"} and {"archived": 1} both persist as true. That makes malformed clients mutate session state instead of receiving a 400, and the response then confirms the unintended value. Please reject non-bool values for both fields (not merely truthy/falsy values) and add regression cases for at least a string and a number, while retaining the existing explicit false toggle coverage.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for closing the API-server session-metadata parity gap; current main still limits PATCH to title/end_reason at gateway/platforms/api_server.py:3241 and omits both fields from _session_response at gateway/platforms/api_server.py:3023.

Problems

  • apps/desktop/src/store/session-pin-sync.ts:86 is unreachable. The existing guard at lines 80-81 already continues when an outstanding false write conflicts with row.pinned === true, so awaited === false cannot reach the new branch.
  • The new mocked endpoint tests do not cover the changed pinned-page behavior at gateway/platforms/api_server.py:3093. Please add a real-SessionDB API test alongside the existing session endpoint fixture in tests/gateway/test_session_api.py:14-51, including an out-of-page pinned row and false-value round trips.

Suggested changes

  • Remove the redundant desktop guard and cover the API-server list/PATCH contract through the real SessionDB fixture.

Automated hermes-sweeper review.

@@ -82,6 +82,10 @@ function pullRemotePins(): void {
}

if (row.pinned && !heldLocally) {
// If we just issued an unpin for this id, wait for it to settle.
if (awaited !== undefined && awaited === false) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This condition is unreachable: the existing check immediately above continues whenever awaited === false and row.pinned === true. Please remove this redundant branch.

@@ -2618,3 +2620,87 @@ def __init__(self, **kwargs):
assert captured[1]["model"] == "minimax/minimax-m3"


# ---------------------------------------------------------------------------
# PATCH /api/sessions/{session_id} — pinned / archived metadata
# ---------------------------------------------------------------------------

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add this endpoint coverage to tests/gateway/test_session_api.py using its real SessionDB fixture, and exercise the changed include_pinned=True list behavior. These mocks verify handler calls but not SessionDB persistence or pinned-row back-fill.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 1, 2026
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Aug 1, 2026
@SongotenU
SongotenU requested review from pestoura and teknium1 August 1, 2026 08:20
…ATCH

The desktop pin/unpin + archive surfaces PATCH /api/sessions/{id} with
{"pinned"} and {"archived"}, but the gateway rejected both fields
(unsupported_session_field 400) and never returned them in the session
payload, so session-pin-sync.ts pull/reconcile passes saw stale state.

- Accept pinned/archived in PATCH allowed fields and persist them
- Include pinned/archived in the client-safe session response keys
- Pass include_pinned=True when listing sessions so old pins survive
  pagination instead of silently dropping out of the sidebar
- Guard the pull pass against re-pinning an id we just unpinned, and
  surface unpin failures instead of swallowing them

Adds TestSessionPatchEndpoint covering pin/archive persistence,
unknown-field rejection, and the False (unpin/unarchive) toggle path.
@SongotenU
SongotenU force-pushed the fix/api-session-pin-archive branch from 593bae4 to a102b60 Compare August 1, 2026 08:30
@SongotenU

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews — both points addressed.

@pestoura — strict boolean validation (done):
_handle_patch_session now rejects non-bool pinned/archived with 400 invalid_field_type instead of bool(...) coercion ({"pinned": "false"} previously persisted True). Regression coverage added for string, int, and float values in tests/gateway/test_api_server.py (TestSessionPatchEndpoint), and explicit false toggle coverage is retained.

@teknium1 — dead desktop guard (done):
Removed the redundant awaited === false guard in pullRemotePins — you're right that the existing awaited !== row.pinned guard at the top of the loop already continues that case. The console.warn on unpin failure is kept since it's a separate, reachable path.

@teknium1 — real-SessionDB API tests (done):
Added to tests/gateway/test_session_api.py using the existing session_db/adapter fixtures:

  • test_list_sessions_backfills_out_of_page_pinned_row — pins the oldest of 5 sessions, asserts a limit=2 page still surfaces it (covers include_pinned at the API layer).
  • test_patch_pinned_archived_round_trips_through_real_db — true→false round trip through a real SQLite DB, asserting the response coerces back to real booleans.

This exposed a real bug the mocked tests were hiding: SQLite stores these flags as 0/1, so the response was leaking pinned: 0 instead of false — which would silently defeat the desktop's typeof row.pinned === 'boolean' guard. _session_response now coerces both fields to bool before serializing.

Verification: scripts/run_tests.sh 120/120 green (both gateway test files), vitest 11/11, tsc --noEmit clean, ruff check clean.

@pestoura pestoura left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Follow-up on the boolean-coercion finding: the current head resolves it. Both PATCH fields now require an actual JSON boolean before persistence, malformed string/number values return 400 invalid_field_type, and the explicit false path remains covered. The added real-SessionDB round-trip also verifies that SQLite's 0/1 representation is serialized back as JSON booleans. CI run 30691886241 completed successfully on this head. No remaining concern from my previous review.

@OutThisLife

Copy link
Copy Markdown
Collaborator

Superseded by #80711. PATCH /api/sessions/{id} now accepts pinned and archived and persists them through set_session_pinned / set_session_archived.

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

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants