feat(handler): unify ?server= / ?box_id= via resolveActiveBox helper (#134)

* feat(handler): unify ?server= / ?box_id= via resolveActiveBox helper (#112)

Three resolution conventions co-existed pre-#112 Phase 4:

  1. `?server=<id>` — gear-settings deep links and the OS-Updates page.
  2. `?box_id=<id>` — InjectIntegrationStatus middleware (the
     header-pill URL persistence).
  3. `gearbox_active_box` cookie — the sticky pill selection.

Handlers that wanted to honor the pill had to write the same
"check ?server=, then cookie, then default" cascade. Handlers that
followed the middleware convention got `?box_id=` for free but lost
gear-settings links. The resolveBoxIDFromRequest helper only knew
`?server=`.

Changes:

  - resolveBoxIDFromRequest now accepts BOTH `?server=` and `?box_id=`
    as synonyms (precedence: server > box_id > cookie > default).
    Handlers can use whichever name fits the surrounding code without
    losing pill-aware routing.
  - New resolveActiveBox(r) returns the full *models.BoxConfig along
    with a presence boolean, so handlers that need an agent URL or
    APIKey don't have to chase a separate getServerConfig call. Folds
    the "single helper for active box" invariant from the issue
    proposal into one call site.

Four tests cover the new contract: ?server= path, ?box_id= path,
precedence when both supplied, and the full-BoxConfig return.

Phase 4 of #112.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(handler): address Copilot review on resolveActiveBox docstring (#112)

Copilot flagged that the docstring for resolveActiveBox claimed
(nil, false) for the "all-boxes / no-box context", but
resolveBoxIDFromRequest falls back to getDefaultServerID() and
getServerConfig accepts entries from the static h.servers list
regardless of DB-enabled state — so any time at least one server
is configured, this helper resolves to one.

Update the docstring to match actual behavior: (nil, false) only
fires when no servers are configured at all OR when an explicit
?server= / ?box_id= references a missing box. Handlers that need
to distinguish "no active box" from "first enabled box" should
consult the auth-context active-box set by InjectIntegrationStatus,
not call this helper.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: sarg3nt

gearbox/internal/framework/handler/handler.go

@@ -287,19 +287,29 @@ func (h *Handler) getDefaultServerID() string {
 }
 
 // resolveBoxIDFromRequest picks the box ID to operate on, in priority:
-//  1. `?server=<id>` query param — explicit per-link override.
+//  1. `?server=<id>` or `?box_id=<id>` query param — explicit per-link
+//     override. Both names accepted as synonyms so handlers can use
+//     whichever convention the surrounding code prefers; the middleware
+//     in InjectIntegrationStatus uses `?box_id=` for URL-persisted
+//     pill switches, while older gear-settings links use `?server=`.
 //  2. `gearbox_active_box` cookie — the header pill's selection.
 //  3. The first enabled server — first-login fallback.
 //
 // Previously the Gears settings handlers only consulted `?server=` and fell
 // straight through to the first server when it was missing, which made the
 // page show the first box's gears even while the header pill was on a
 // different box (issue #71 item 1). Reading the cookie aligns these
-// handlers with the pill that's actually visible to the user.
+// handlers with the pill that's actually visible to the user. Recognizing
+// `?box_id=` as a synonym keeps the box-resolver consistent across the
+// dashboard's handler / middleware / gear-plugin layers (issue #112
+// Phase 4).
 func (h *Handler) resolveBoxIDFromRequest(r *http.Request) string {
 	if id := r.URL.Query().Get("server"); id != "" {
 		return id
 	}
+	if id := r.URL.Query().Get("box_id"); id != "" {
+		return id
+	}
 	if c, err := r.Cookie(activeBoxCookieName); err == nil && c.Value != "" {
 		// Only honor the cookie if it still resolves to an enabled server —
 		// stale cookies (deleted/disabled boxes) shouldn't dictate behavior.
@@ -312,6 +322,38 @@ func (h *Handler) resolveBoxIDFromRequest(r *http.Request) string {
 	return h.getDefaultServerID()
 }
 
+// resolveActiveBox returns the full BoxConfig for the box the request is
+// acting on, using resolveBoxIDFromRequest for resolution. Returns
+// (nil, false) when:
+//
+//   - There are no servers configured at all (resolveBoxIDFromRequest's
+//     getDefaultServerID fallback has nothing to return), OR
+//   - The resolved ID doesn't match any entry in the static
+//     h.servers list or the database — e.g. a stale link with
+//     ?server=<deleted-box-id>.
+//
+// Note: this does NOT return (nil, false) for an "all-boxes
+// dashboard context". When at least one server is configured,
+// resolveBoxIDFromRequest falls back to the first enabled box, and
+// getServerConfig accepts entries from the static h.servers list
+// whether or not they're DB-enabled, so any time at least one
+// server exists this helper resolves to one. Handlers that need to
+// distinguish "no active box" from "first enabled box" should
+// consult the auth-context active-box set by
+// InjectIntegrationStatus, not call this helper.
+//
+// Handlers that need an agent client, an agent URL, or other
+// BoxConfig fields should prefer this over resolveBoxIDFromRequest +
+// a separate getServerConfig call: it folds the existence check
+// into one call site (issue #112 Phase 4).
+func (h *Handler) resolveActiveBox(r *http.Request) (*models.BoxConfig, bool) {
+	boxID := h.resolveBoxIDFromRequest(r)
+	if boxID == "" {
+		return nil, false
+	}
+	return h.getServerConfig(boxID)
+}
+
 // activeBoxCookieName is the cookie key that persists the user's selected
 // box across navigations. Lets gear links (e.g. /haproxy) drop the verbose
 // `?box_id=` query string and still resolve the active context.

gearbox/internal/framework/handler/resolve_box_test.go

@@ -0,0 +1,79 @@
+package handler
+
+import (
+	"net/http/httptest"
+	"testing"
+
+	"github.com/sarg3nt/gearbox/internal/framework/models"
+)
+
+// newResolveTestHandler builds the minimum Handler state the resolver
+// needs for the URL-query-param paths: a static `servers` slice (so the
+// cookie path can validate against enabled servers if exercised). Tests
+// that exercise the cookie or first-server fallback would also need a
+// DB; those branches aren't covered here.
+func newResolveTestHandler(servers ...models.BoxConfig) *Handler {
+	return &Handler{servers: servers}
+}
+
+// TestResolveBoxIDFromRequest_ServerParam covers the historical
+// `?server=<id>` path — gear-settings links and the OS-Updates page
+// still emit this form.
+func TestResolveBoxIDFromRequest_ServerParam(t *testing.T) {
+	h := newResolveTestHandler()
+	r := httptest.NewRequest("GET", "/anything?server=mjolnir", nil)
+	if got := h.resolveBoxIDFromRequest(r); got != "mjolnir" {
+		t.Errorf("resolveBoxIDFromRequest with ?server=mjolnir = %q, want %q", got, "mjolnir")
+	}
+}
+
+// TestResolveBoxIDFromRequest_BoxIDParam covers the synonym alias added
+// in issue #112 Phase 4. `?box_id=` is the form the header-pill
+// middleware writes; pre-Phase-4 the handler-level resolver ignored it
+// because it only knew `?server=`. Recognizing both keeps the resolver
+// consistent across the handler / middleware / gear-plugin layers.
+func TestResolveBoxIDFromRequest_BoxIDParam(t *testing.T) {
+	h := newResolveTestHandler()
+	r := httptest.NewRequest("GET", "/anything?box_id=mjolnir", nil)
+	if got := h.resolveBoxIDFromRequest(r); got != "mjolnir" {
+		t.Errorf("resolveBoxIDFromRequest with ?box_id=mjolnir = %q, want %q", got, "mjolnir")
+	}
+}
+
+// TestResolveBoxIDFromRequest_ServerWinsOverBoxID is the precedence
+// guard: when a request happens to carry both (rare — typically only
+// when an operator hand-edits a URL), `?server=` wins because it's the
+// older, more-explicit convention used by gear-settings deep links.
+func TestResolveBoxIDFromRequest_ServerWinsOverBoxID(t *testing.T) {
+	h := newResolveTestHandler()
+	r := httptest.NewRequest("GET", "/anything?server=alpha&box_id=beta", nil)
+	if got := h.resolveBoxIDFromRequest(r); got != "alpha" {
+		t.Errorf("resolveBoxIDFromRequest with both params = %q, want %q", got, "alpha")
+	}
+}
+
+// TestResolveActiveBox_ReturnsServerConfig verifies that the new
+// resolveActiveBox returns the full BoxConfig (not just an ID) by
+// matching against the handler's static `servers` slice.
+func TestResolveActiveBox_ReturnsServerConfig(t *testing.T) {
+	h := newResolveTestHandler(
+		models.BoxConfig{ID: "mjolnir", Name: "Mjolnir", AgentURL: "https://10.0.0.1:8405"},
+	)
+	r := httptest.NewRequest("GET", "/anything?box_id=mjolnir", nil)
+	box, ok := h.resolveActiveBox(r)
+	if !ok {
+		t.Fatalf("resolveActiveBox = (_, false), want a BoxConfig")
+	}
+	if box.ID != "mjolnir" || box.AgentURL != "https://10.0.0.1:8405" {
+		t.Errorf("resolveActiveBox returned %+v, want {ID:mjolnir, AgentURL:https://10.0.0.1:8405}", box)
+	}
+}
+
+// The "resolved ID doesn't match any configured server" branch isn't
+// covered here because exercising it would require a DB stub —
+// getServerConfig falls back to getEnabledServers (a DB read) when the
+// static list misses, and the DB layer panics on nil. The static-list
+// happy path above guards the helper's primary contract (an ID that
+// matches a configured server returns the right BoxConfig); the
+// dynamic-server path is exercised by existing handler integration
+// tests that run against the full Handler fixture.