feat(notion): add v1 OAuth backend alongside v3 cookie path

[daltoniam]

Summary

The current notion integration talks to www.notion.so/api/v3 with a browser session cookie (token_v2). That path still works for users who set it up before OAuth existed, but it's brittle (cookies expire, Notion can change v3 without notice) and unusable for hosted SaaS.

This adds a parallel notionV1 backend that uses Notion's documented public REST API (api.notion.com/v1) with an OAuth bearer token and the date-stamped Notion-Version header. I picked 2025-09-03 — that's the version that introduced the data_sources model, so the public surface matches the v3 backend's database/data-source split naturally and we don't need a translation layer.

Backend selection happens in Configure() based on which credential key is set:

• access_token → v1 (OAuth, hosted-friendly)
• token_v2 → v3 (cookie, self-host legacy)

If both are present, v1 wins. Execute() and Healthy() delegate to v1 when it's configured; otherwise the existing v3 path runs unchanged. No behavior change for existing token_v2 users.

All tools the v3 backend exposes have a v1 equivalent (data sources, pages, blocks, search, users, comments, plus the convenience helpers notion_get_page_content / notion_create_page_with_content). Routing lives in dispatchV1. If a tool ends up v3-only in the future, Execute returns a clear "not supported on the Notion OAuth (v1) backend" error rather than a generic unknown-tool error.

Test plan

• go test ./integrations/notion/... — new v1_test.go covers the configure handshake (auth + version headers), and each handler family with httptest servers asserting URL paths, request bodies, and response shaping.
• go test ./... — full suite green.
• golangci-lint run ./integrations/notion/... — clean.
• Smoke-test end-to-end against a real Notion OAuth install (wired in Switchboard-hosted, separate PR).

[acmacalister]

Good stuff overall — the dual-backend design is clean, the test coverage is thorough, build and lint are both green. Four things worth tidying up inline.

[acmacalister]

dispatchV1 is missing the two parity tests every dispatch map in this project is required to have. The v3 dispatch has TestDispatchMap_EveryToolHasHandler and TestDispatchMap_NoOrphanHandlers in notion_test.go — dispatchV1 needs equivalents.

Right now, nothing enforces that every entry in dispatchV1 has a corresponding ToolDefinition in Tools(), or vice versa. If a tool name is misspelled here or a new tool is added to Tools() without a v1 handler, it silently routes to the "not yet supported on v1 backend" error at runtime instead of failing at test time.

func TestDispatchV1_AllToolsCovered(t *testing.T) {
    i := New()
    for _, tool := range i.Tools() {
        _, ok := dispatchV1[tool.Name]
        assert.True(t, ok, "tool %s has no v1 dispatch handler", tool.Name)
    }
}

func TestDispatchV1_NoOrphanHandlers(t *testing.T) {
    i := New()
    toolNames := make(map[mcp.ToolName]bool)
    for _, tool := range i.Tools() {
        toolNames[tool.Name] = true
    }
    for name := range dispatchV1 {
        assert.True(t, toolNames[name], "v1 dispatch handler %s has no tool definition", name)
    }
}

[daltoniam]

Done in 785138e — added both TestDispatchV1_EveryToolHasHandler and TestDispatchV1_NoOrphanHandlers in v1_test.go, matching the v3 pattern.

[acmacalister]

This fallback triggers on any error from /data_sources/, including transient ones. If Notion rate-limits the request (429), doRequest returns a *mcp.RetryableError — but instead of propagating it so the runtime can back off, we swallow it and immediately fire a second request to /databases/. Same issue exists in v1QueryDataSource (line 164) and v1FetchDatabaseOrDataSource (line 199).

The fallback only makes sense for 404-style "this ID is not a data source" responses. A guard like this preserves the intent while letting retryable errors escape:

data, err := n.patch(ctx, "/data_sources/"+id, body)
if err != nil {
    var re *mcp.RetryableError
    if errors.As(err, &re) {
        return mcp.ErrResult(err)
    }
    data, err = n.patch(ctx, "/databases/"+id, body)
    if err != nil {
        return mcp.ErrResult(err)
    }
}

[daltoniam]

Good catch. Fixed in 785138e — added an errors.As guard on *mcp.RetryableError to all three fallback sites (v1UpdateDataSource, v1QueryDataSource, v1FetchDatabaseOrDataSource). Rate-limits and 5xx now propagate so the runtime can back off; only non-retryable errors fall through to the /databases/ retry.

[acmacalister]

Both branches of v1TitleKeyForParent return "title", so the condition is never evaluated. The comment above acknowledges that database rows use a user-defined title column name, but the code doesn't implement that distinction — it just returns "title" unconditionally.

Either inline the literal "title" at the call site and delete this function, or implement the differentiation the comment describes. As written, the function misleads readers into thinking there's conditional logic here.

[daltoniam]

You're right, the function was dead. Removed in 785138e and inlined const titleKey = "title" at the call site, with the comment updated to reflect what actually happens (default to title; callers can override via the properties map for database rows with a renamed title column).

[acmacalister]

jsonUnmarshalLite is a one-liner wrapper around json.Unmarshal that adds no behaviour and is only called once (in v1FetchBlockType). Worth replacing the call site with json.Unmarshal directly and removing this.

[daltoniam]

Removed in 785138e. v1FetchBlockType now calls json.Unmarshal directly; encoding/json moved into v1_blocks.go and out of v1_other.go.

[acmacalister]

Solid PR — build passes, tests pass, lint clean. The retryable-error guards and dispatch parity tests are both in good shape. Three things inline, two of which are about test coverage.

[acmacalister]

v1UpdateBlock is the most complex handler in this PR — it makes an extra GET /blocks/{id} to discover the block type, then translates the v3 properties.title shape to a v1 rich_text payload. The passedThrough detection (skip "properties"/"format", treat anything else as a v1 payload) has branching that's easy to accidentally break.

Every other non-trivial handler has at least one httptest case; this one doesn't. The dispatch parity tests confirm it's wired up, but they don't exercise any of the translation logic. Worth adding cases for at least the v3 legacy path (where v1FetchBlockType is called) and the v1 pass-through path.

[acmacalister]

The errors.As guard here is the right fix, but there's no test proving a 429 from /data_sources/ actually stops the fallback. TestV1_QueryDataSourceFallsBackToDatabases only exercises the 404 path — if the guard is ever removed, the suite stays green.

Something like:

func TestV1_QueryDataSourceDoesNotFallBackOn429(t *testing.T) {
    callCount := 0
    v1, _ := newV1WithServer(t, func(w http.ResponseWriter, r *http.Request) {
        callCount++
        w.Header().Set("Retry-After", "5")
        w.WriteHeader(http.StatusTooManyRequests)
        _, _ = w.Write([]byte(`{"object":"error","code":"rate_limited","message":"slow down"}`))
    })
    _, err := v1QueryDataSource(context.Background(), v1, map[string]any{"data_source_id": "ds-1"})
    require.Error(t, err)
    var re *mcp.RetryableError
    require.ErrorAs(t, err, &re)
    assert.Equal(t, 1, callCount, "429 should not trigger the /databases/ fallback")
}

Same test is worth having for v1UpdateDataSource and v1FetchDatabaseOrDataSource (used by v1RetrieveDataSource).

[acmacalister]

The > 100 guard fires after v1BuildPageBody has already normalized all N children and constructed the full body. No API call is made so the behavior is correct, but for a 200-child input we're doing twice the normalization work before surfacing the error. Moving the check before the v1BuildPageBody call (or even right after the normalization loop) keeps validation upfront:

if len(childrenAny) > 100 {
    return mcp.ErrResult(fmt.Errorf("v1 create_page_with_content accepts up to 100 children..."))
}
children := make([]map[string]any, 0, len(childrenAny))
...
body, err := v1BuildPageBody(...)

[arjansingh]

Existing code looks fine. it's worth adding compact.yml specs because IIRC, even the v1 api has some extraneous info.

[daltoniam]

Done in 4eb2ba7 — added compact_v1.yaml with v1-shape specs and wired CompactSpec / MaxBytes / Views to switch on n.v1 != nil so the v3 specs (parent_id flat, properties.title flat string) keep working on the cookie backend while v1 gets its own paths (nested parent.{type,page_id}, typed properties map, etc.).

You were right about the noise — drops created_by / last_edited_by full user objects, cover, icon, public_url, request_status, the is_archived / is_locked duplicates, the deprecated archived alias, and the object / type wrappers. Multi-view tools (search, query_data_source, retrieve_data_source, get_page_content, retrieve_comments) get the same slim-default-+-full shape as v3.

Backed by a TestV1FieldCompactionSpecs_ParityWithV3 test that fails if any tool is in one yaml but not the other, plus mirrored versions of all four structural tests (AllParse, NoDuplicateTools, NoOrphanSpecs, NoMutationTools) and switch-on-backend assertions for CompactSpec and Views.

One known gap: views are JSON-only on v1 for now — the existing markdown renderers are v3 record-map shaped and would mis-render v1 blocks. A v1 markdown renderer is worth a follow-up PR but felt out of scope here.

[acmacalister]

Build clean, full test suite green (race detector), lint at 0. The dual-backend design is well-executed and the parity/compaction tests are solid additions.

Two items from acmacalister's fixup review are still outstanding before merge: the missing 429-stops-fallback tests for v1QueryDataSource, v1UpdateDataSource, and v1FetchDatabaseOrDataSource (discussion_r3335291076), and the children > 100 guard placement in v1CreatePageWithContent (discussion_r3335291085). Both are real — I agree with that feedback. Nothing new blocking from this pass.