feat(connector): [YW-263] auth-blind pipeline via SecretResolver bound-resolver#131
Merged
youhaowei merged 3 commits intoJun 17, 2026
Conversation
📝 WalkthroughWalkthroughThe PR shifts Notion connector operations from client-side singleton to a server-side factory pattern. A new ChangesNotion Server-Side Data-Plane with Vault-Backed SecretResolver
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer UI
participant AppData as app-data (useNotionMutations)
participant Server as Server (appArtifactFunctions)
participant Vault as SecretVault
participant NotionAPI as Notion API
rect rgba(100, 149, 237, 0.5)
Note over Renderer,NotionAPI: List Databases Flow
Renderer->>AppData: notionMutations.listDatabases(dataSourceId)
AppData->>Server: api.listNotionDatabases(dataSourceId)
Server->>Vault: mintBoundResolver → vault.withSecret(ref)
Vault-->>Server: bound SecretResolver
Server->>NotionAPI: connector.connect() resolves API key via auth
NotionAPI-->>Server: RemoteDatabase[]
Server-->>AppData: NotionDatabaseRef[] {id, title}
AppData-->>Renderer: NotionDatabaseRef[]
end
rect rgba(144, 238, 144, 0.5)
Note over Renderer,NotionAPI: Query Database Flow
Renderer->>AppData: notionMutations.queryDatabase(dataSourceId, databaseId, tableId)
AppData->>Server: api.queryNotionDatabase(...)
Server->>Vault: mintBoundResolver → vault.withSecret(ref)
Vault-->>Server: bound SecretResolver
Server->>NotionAPI: connector.query(databaseId, tableId) resolves API key via auth
NotionAPI-->>Server: Arrow IPC data
Server-->>AppData: NotionQueryResult {arrowBuffer, fieldIds, fields}
AppData-->>Renderer: NotionQueryResult
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a313c3952a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…solver
Inverts the connector credential contract so the pipeline is auth-blind:
- Define SecretResolver type in @dashframe/engine (capability-attenuated
bound lease: <T>(use: (plaintext: string) => Promise<T>) => Promise<T>)
- RemoteApiConnector constructor takes auth: SecretResolver; connect() and
query() carry no credential arguments
- FileSourceConnector.parse() removes the unused optional formData arg
- NotionConnector uses this.auth(use => ...) internally; exported via
makeNotionConnector(auth) factory (singleton removed)
- mintBoundResolver(vault, ref, label) in server/app-artifacts.ts is the
single mint site: (vault, ref) → boundResolver → connector
- listNotionDatabases WyStack mutation resolves the credential via vault;
the handler has no plaintext in scope
- Renderer registers a metadata-only connector with a throwing resolver;
connect()/query() route through server mutations, never the renderer
- queryNotionDatabase deferred: NotionConnector.query() calls
DataFrame.create({storageType:indexeddb}) which requires a browser
environment; the server-safe query path belongs in a follow-on
- Tests: resolver-count oracle for both connect() and query(); two
mintBoundResolver fail-closed tests (no-vault + plaintext-not-ref)
added to vault-control-plane.test.ts; DataFrame.create mocked in
connector-notion tests (Node environment)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ugh server
Round 2 — resolves the open review threads and un-defers the query path.
Engine + connector contract:
- ConnectorQueryResult is now serializable: { arrowBuffer, fieldIds, fields }
instead of a live DataFrame. This is the root unblock — a live DataFrame
binds query() to a browser (IndexedDB) and can't cross IPC. NotionConnector
.query() now returns the raw Arrow buffer + ids and builds no DataFrame, so
it runs cleanly in a Node server handler. The renderer materializes the
browser DataFrame from the result.
Server (app-artifacts.ts):
- queryNotionDatabase mutation un-deferred — resolves the credential via the
bound resolver inside connector.query() and returns the serializable result.
- Shared notionConnectorFor(ctx, dataSourceId) seam: row lookup + kind guard +
bound-resolver mint + connector construction, used by both notion routes.
- listNotionDatabases maps the connector's {id,name} to the renderer DTO
{id,title} so DataSourceControls renders/adds by title without a mismatch.
- Import the canonical SecretResolver from @dashframe/engine (aliased
BoundSecretResolver) instead of re-declaring the type; added @dashframe/engine
as an explicit dep.
Renderer routing (the credential surface):
- ConnectorCardWithForm no longer calls connector.connect()/query(). It
validates the form and hands the credentials up; the credential resolves
SERVER-SIDE. onConnect now carries credentials, not a database list.
- DataSourceControls lists databases via useNotionMutations().listDatabases
(new app-data hook over api.listNotionDatabases) — not the dead trpc stub.
- DataSourceDisplay's schema/property/preview flow (a separate NOTION_ENABLED
feature) short-circuits before any server call; removed its dead trpc stubs.
- Deleted the orphaned trpc Provider stub.
Tests:
- query() asserts the serializable contract + resolveCallCount.
- queryNotionDatabase fail-closed (no-vault, wrong-kind) tests.
- New notion-routes happy-path tests: name→title mapping + serializable query
result + resolver invoked once + no plaintext in the payload (mocked client).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
youhaowei
force-pushed
the
charixandra/yw-263-secretvault-data-plane-bound-resolver-remoteapiconnector
branch
from
a313c39 to
025b456
Compare
Owner
Author
Round 2 — resolved all review threads + closed the query pathThanks for the reviews. All four findings are addressed in P1 (Greptile) + P2 (Codex) — renderer calling P2 (Codex) — preserve the P2 (Greptile) — inlined Closed the deferred query path (the heart of the ticket) Out of scope (follow-up): the frozen Full local gate (turbo typecheck + lint + test, 85 tasks) green before push. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/app/src/components/data-sources/AddConnectionPanel.tsx (1)
34-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the example callback to use credentials, not databases.
The example still shows the old
onConnectpayload, which now conflicts with the updated prop contract and can mislead future integrations.Suggested doc fix
- * onConnect={(connector, databases) => handleConnect(databases)} + * onConnect={(connector, credentials) => handleConnect(credentials)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/components/data-sources/AddConnectionPanel.tsx` around lines 34 - 38, The JSDoc example for the AddConnectionPanel component shows an outdated onConnect callback signature that receives databases as a parameter, but the actual prop contract now expects credentials instead. Update the example callback in the comment block to show onConnect receiving (connector, credentials) instead of (connector, databases) to align with the current implementation and prevent confusion for future integrations.packages/app/src/hooks/useConnectorForm.ts (1)
17-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the stale example to reflect
connect()’s current signature.The example still calls
notionConnector.connect(data), but the updated contract resolves credentials internally andconnect()no longer takes form data.Suggested doc fix
- * const databases = await execute((data) => notionConnector.connect(data)); + * const databases = await execute(async () => notionConnector.connect());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/hooks/useConnectorForm.ts` around lines 17 - 21, The example code for handleConnect in the useConnectorForm hook shows an outdated call to notionConnector.connect(data), but the connect() method has been updated to no longer accept form data as a parameter since it now resolves credentials internally. Update the function call to remove the data argument being passed to connect(), so it simply calls notionConnector.connect() without any parameters, to reflect the current method signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app-data/src/notion.ts`:
- Around line 22-26: The fields property in the NotionQueryResult interface
currently uses unknown[] as its type, which compromises type safety for the
public API contract. Replace the unknown[] type with the appropriate shared
field type definition used elsewhere in the codebase (typically a union or
concrete type representing valid field objects). This will ensure that renderer
and server implementations are checked at compile time for consistency rather
than using the overly permissive unknown type.
In `@packages/app/src/components/data-sources/DataSourcesWorkbench.tsx`:
- Around line 279-288: The handleRemoteConnect function currently only logs and
displays a toast message when a remote connector form is submitted, but does not
actually create a data source or persist the credentials, resulting in a broken
user workflow. Either implement the complete remote-source creation path by
calling the appropriate mutation to create the DataSource (storing credentials
as a vault SecretRef) and handling the response, or alternatively disable/hide
the remote connector submission option in the UI until the full implementation
is ready. Choose one approach based on your team's priorities for completing
this feature.
In `@packages/connector-notion/src/connector.test.ts`:
- Around line 209-219: The test for makeNotionConnector does not actually
validate the claimed TypeScript compile-time contract that the auth parameter is
required. Add a test case within the same it block that attempts to call
makeNotionConnector without any arguments and mark it with `@ts-expect-error`.
This ensures TypeScript prevents the call at compile time and guards against
accidental regression if the auth parameter becomes optional in the type
definition. The existing valid resolver test case can remain to demonstrate the
correct usage.
---
Outside diff comments:
In `@packages/app/src/components/data-sources/AddConnectionPanel.tsx`:
- Around line 34-38: The JSDoc example for the AddConnectionPanel component
shows an outdated onConnect callback signature that receives databases as a
parameter, but the actual prop contract now expects credentials instead. Update
the example callback in the comment block to show onConnect receiving
(connector, credentials) instead of (connector, databases) to align with the
current implementation and prevent confusion for future integrations.
In `@packages/app/src/hooks/useConnectorForm.ts`:
- Around line 17-21: The example code for handleConnect in the useConnectorForm
hook shows an outdated call to notionConnector.connect(data), but the connect()
method has been updated to no longer accept form data as a parameter since it
now resolves credentials internally. Update the function call to remove the data
argument being passed to connect(), so it simply calls notionConnector.connect()
without any parameters, to reflect the current method signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b146b57-acbd-47a4-85be-36463c07d266
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
apps/server/package.jsonapps/server/src/functions/app-artifacts.tsapps/server/src/functions/notion-routes.test.tsapps/server/src/functions/vault-control-plane.test.tspackages/app-data/src/index.tspackages/app-data/src/notion.tspackages/app/src/components/data-sources/AddConnectionPanel.tsxpackages/app/src/components/data-sources/DataPickerContent.tsxpackages/app/src/components/data-sources/DataSourceControls.tsxpackages/app/src/components/data-sources/DataSourceDisplay.tsxpackages/app/src/components/data-sources/DataSourcesWorkbench.tsxpackages/app/src/components/data-sources/renderers/ConnectorCardWithForm.tsxpackages/app/src/components/providers/ConnectorSetup.tsxpackages/app/src/hooks/useConnectorForm.tspackages/app/src/lib/connectors/registry.test.tspackages/app/src/lib/trpc/Provider.tsxpackages/connector-notion/package.jsonpackages/connector-notion/src/connector.test.tspackages/connector-notion/src/connector.tspackages/connector-notion/src/index.tspackages/engine/src/connector/base.tspackages/engine/src/connector/index.tspackages/engine/src/connector/types.tspackages/engine/src/index.ts
💤 Files with no reviewable changes (1)
- packages/app/src/lib/trpc/Provider.tsx
…contract test - NotionQueryResult.fields: unknown[] → Field[] (typed end-to-end contract) - connector test: @ts-expect-error on makeNotionConnector() makes AC #1's "auth is required" compile-time contract executable (fails if auth ever goes optional) - refresh stale JSDoc examples in AddConnectionPanel + useConnectorForm to the credentials-based onConnect / no-arg connect() contract Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
youhaowei
deleted the
charixandra/yw-263-secretvault-data-plane-bound-resolver-remoteapiconnector
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RemoteApiConnector.connect()andquery()carry no credential arguments; the constructor takesauth: SecretResolver(a capability-attenuated lease pre-bound to oneSecretRef)SecretResolver = <T>(use: (plaintext: string) => Promise<T>) => Promise<T>in@dashframe/engine; exported from the engine entry pointNotionConnectorusesthis.auth(use => ...)internally; singleton removed, replaced bymakeNotionConnector(auth)factorymintBoundResolver(vault, ref, label)inapps/server/src/functions/app-artifacts.tsis the single mint site:(vault, ref) → boundResolver → connectorlistNotionDatabasesWyStack mutation reads the DataSource row, extracts theSecretReffrom config, mints a resolver, and callsconnector.connect()— no plaintext in scope at the call siteconnect()/query()route through server mutations, never the renderer IPCqueryNotionDatabaseis deferred:NotionConnector.query()callsDataFrame.create({storageType:"indexeddb"})which requires a browser environment; the server-safe query path belongs in a follow-on (see deferred comment in source)FileSourceConnector.parse()removes the dead optionalformDataargTest coverage
connector.test.ts: resolver-count oracle for bothconnect()(existing) andquery()(new);DataFrame.createmocked in Node environmentvault-control-plane.test.ts: two newmintBoundResolverfail-closed tests — no-vault path and plaintext-not-SecretRef path — exercised throughlistNotionDatabasesGate
turbo typecheck: 44/44 passturbo test: connector-notion 73/73, server 191/191, app 473/473, all others passturbo lint: 18/18 passTracked internally as YW-263.
🤖 Generated with Claude Code
Greptile Summary
This PR inverts the connector credential contract:
RemoteApiConnector.connect()andquery()no longer accept credential arguments; instead the constructor takes aSecretResolvercapability-attenuated to oneSecretRef. The renderer-registered connector gets a throwing resolver (metadata-only), while the server mints a bound resolver per DataSource row before constructing the connector. The tRPC stub (Provider.tsx) is deleted, and two new WyStack mutations (listNotionDatabases,queryNotionDatabase) replace the old tRPC routes.ConnectorQueryResultis also reshaped to be IPC-serializable (arrowBuffer + fieldIds + fieldsinstead of a liveDataFrame).SecretResolverdefined in@dashframe/engine,RemoteApiConnectorbase class now requires it in its constructor;NotionConnectorcallsthis.auth(use => …)internallymintBoundResolver(vault, ref, label)inapp-artifacts.tsis the single seam; fail-closed on no-vault and non-SecretRefpaths — both exercised in new testsConnectorCardWithFormnow callsexecute(async data => data)(form validation only), passing validated form values up to the stubonConnecthandler; deferred flows inDataSourceDisplayare gated behindNOTION_ENABLED=falsewith explicit comments pointing to the follow-on wiringConfidence Score: 5/5
Safe to merge — the credential-inversion contract is correctly enforced at every boundary: the renderer connector throws on connect/query, the mint site is fail-closed, and both server mutations resolve credentials exclusively through the vault's
withSecretcallback.The auth-blind pipeline is structurally sound:
SecretResolveris defined once and imported from the canonical export,mintBoundResolveris fail-closed on both the no-vault and non-SecretRef paths, andConnectorQueryResultis now IPC-serializable with no liveDataFrame. The tRPC stub is cleanly removed. All deferred code paths are gated behindNOTION_ENABLED=falsewith clear follow-on comments. The test matrix covers happy-path DTO mapping, resolver invocation counts, plaintext absence in responses, and all four fail-closed scenarios.No files require special attention.
DataSourceDisplay.tsxhas intentionally dead code with a type annotation shaped for the old tRPC result; this will need updating when the preview path is wired to the new mutation.Important Files Changed
SecretResolvertype and reworksRemoteApiConnectorto require it in its constructor;connect()andquery()signatures drop credential arguments.FileSourceConnector.parse()optionalformDataarg also removed.mintBoundResolverfactory (fail-closed),notionConnectorForhelper, and two new mutationslistNotionDatabases/queryNotionDatabase; imports reorganized to bring in@dashframe/connector-notionand@dashframe/engine.makeNotionConnector(auth)factory added;connect()andquery()usethis.auth(use => …)with no credential param;query()no longer creates a DataFrame (serializable result only).execute(async data => data)— validates form and returns credentials to parent;connector.connect()is never called from the renderer.onConnectsignature updated to acceptRecord<string, unknown>instead ofRemoteDatabase[].pendingQueryResult()/pendingSchema()guards that throw, each gated behind!NOTION_ENABLEDshort-circuits. Dead code path preserved for future wiring.useNotionMutationshook and imperativelistNotionDatabaseshelper; both call server-side mutations via@wystack/client, withascasts on mutation return values.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant R as Renderer participant F as ConnectorCardWithForm participant SM as WyStack Mutation participant MBR as mintBoundResolver participant V as SecretVault participant NC as NotionConnector R->>F: user submits form (apiKey) F->>F: "execute(async data => data) validates" F-->>R: onConnect(connector, credentials) stub Note over SM,NC: Server-side path R->>SM: "listNotionDatabases({dataSourceId})" SM->>MBR: mintBoundResolver(vault, ref, label) MBR->>MBR: fail-closed checks MBR-->>SM: boundResolver SM->>NC: makeNotionConnector(boundResolver) SM->>NC: connector.connect() NC->>V: "this.auth(use => listDatabases(apiKey))" V-->>NC: plaintext (inside callback only) NC-->>SM: RemoteDatabase[] SM-->>R: "[{id, title}, ...]"%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant R as Renderer participant F as ConnectorCardWithForm participant SM as WyStack Mutation participant MBR as mintBoundResolver participant V as SecretVault participant NC as NotionConnector R->>F: user submits form (apiKey) F->>F: "execute(async data => data) validates" F-->>R: onConnect(connector, credentials) stub Note over SM,NC: Server-side path R->>SM: "listNotionDatabases({dataSourceId})" SM->>MBR: mintBoundResolver(vault, ref, label) MBR->>MBR: fail-closed checks MBR-->>SM: boundResolver SM->>NC: makeNotionConnector(boundResolver) SM->>NC: connector.connect() NC->>V: "this.auth(use => listDatabases(apiKey))" V-->>NC: plaintext (inside callback only) NC-->>SM: RemoteDatabase[] SM-->>R: "[{id, title}, ...]"Comments Outside Diff (1)
packages/app/src/hooks/useConnectorForm.ts, line 18 (link)connect(data)signature. Sinceconnect()is now auth-blind and takes no arguments, the example should be updated to avoid misleading future callers.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (3): Last reviewed commit: "chore(connector): address review nits — ..." | Re-trigger Greptile
Verification
Local gate (full graph) green before each push:
turbo typecheck lint test— 85 tasks, all passing. Key runtime-behavior coverage:connector.query()returns a serializable{ arrowBuffer, fieldIds, fields }and builds no DataFrame —connector.test.tsasserts the shape,resolveCallCount === 1, andnot.toHaveProperty("dataFrame"). Proves it runs without a browser/IndexedDB.notion-routes.test.tsexerciseslistNotionDatabases/queryNotionDatabaseend-to-end throughcreateWyStack+ aTestBackendvault — verifies the{id,name}→{id,title}mapping, the serializable query payload, resolver-invoked-once, and that no plaintext appears in the response payload.vault-control-plane.test.tscovers no-vault, non-SecretRef config, and wrong-kind — all throw before any network call.connector.test.ts).connect()/query()are not called from the renderer (verified by grep + the throwing-resolver backstop).@ts-expect-error makeNotionConnector()makes the "auth is required" contract executable.The NOTION_ENABLED renderer preview/property-selection UI is gated off (default false) and is a separate follow-on; the credential-inversion contract is what this ticket verifies.