Add FileMaker bridge authorization flow for CLI and typegen#267
Open
eluce2 wants to merge 5 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: 4823a0e The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds FM MCP persistent-token support and session caching, implements interactive local FM MCP authorization in the CLI, adds adapter-side authorize-and-retry for /callScript, threads authorization into typegen/server/UI (including session identity and persistent token resolution), and makes the manifest revalidation secret optional with tests. ChangesManifest Revalidation Secret Handling
Core Authorization and Environment Support
CLI Local FM MCP Authorization
Typegen FM MCP Integration
Release Metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/fmdapi/src/adapters/fm-mcp.ts (1)
333-368:⚠️ Potential issue | 🟠 MajorFix
executeScriptto honor timeout and fetch options.The
executeScriptmethod in fm-mcp adapter ignorestimeoutandfetchoptions, while all other methods (get, create, update, delete, layoutMetadata) pass these tothis.request(). SinceExecuteScriptOptionsextendsBaseRequest(which includes bothtimeout?: numberandfetch?: RequestInit), callers expect these options to work consistently.Update the
executeScriptmethod to passtimeout: opts.timeoutandfetchOptions: opts.fetchto the fetch call, matching the pattern used in other methods.🤖 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/fmdapi/src/adapters/fm-mcp.ts` around lines 333 - 368, The executeScript method currently builds and calls fetch directly (via the postCallScript closure) and thus ignores ExecuteScriptOptions.timeout and ExecuteScriptOptions.fetch; change it to use the same request helper as other methods (pass opts.timeout as timeout and opts.fetch as fetchOptions) or have postCallScript call this.request with { path: "/callScript", method: "POST", timeout: opts.timeout, fetchOptions: opts.fetch, body: { connectedFileName: this.connectedFileName, scriptName: opts.script, data: opts.scriptParam } }; ensure the unauthorized handling still replays the request through the same request helper (refer to executeScript, postCallScript, and this.request).packages/typegen/src/server/createDataApiClient.ts (1)
347-367:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the caller's project root for FM MCP prompt identity.
Line 366 derives the default
clientName/clientDescriptionfromprocess.cwd(), but this flow already has project-aware context viacreateDataApiClient(context, ...). If the UI server is launched from a different directory, the authorization prompt will show the wrong project name.Suggested fix
-export async function createClientFromConfig( - config: FmdapiConfig, +export async function createClientFromConfig( + config: FmdapiConfig, + options?: { cwd?: string }, ): Promise<Omit<CreateClientResult, "config"> | CreateClientError> { + const cwd = options?.cwd ?? process.cwd(); let deps: Awaited<ReturnType<typeof loadFmdapiDeps>>; try { deps = await loadFmdapiDeps(); @@ - const fmMcpClientIdentity = getFmMcpClientIdentity(process.cwd()); + const fmMcpClientIdentity = getFmMcpClientIdentity(cwd);- const result = await createClientFromConfig(config); + const result = await createClientFromConfig(config, { cwd: context.cwd });🤖 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/typegen/src/server/createDataApiClient.ts` around lines 347 - 367, The FM MCP client identity is being derived from process.cwd() which can be the wrong directory; update the code that calls getFmMcpClientIdentity(process.cwd()) inside createDataApiClient to use the project's root from the provided context (e.g., getFmMcpClientIdentity(context.projectRoot) or the appropriate context property) so fmMcpClientIdentity is derived from the caller's project root rather than the server's working directory.
🤖 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.
Outside diff comments:
In `@packages/fmdapi/src/adapters/fm-mcp.ts`:
- Around line 333-368: The executeScript method currently builds and calls fetch
directly (via the postCallScript closure) and thus ignores
ExecuteScriptOptions.timeout and ExecuteScriptOptions.fetch; change it to use
the same request helper as other methods (pass opts.timeout as timeout and
opts.fetch as fetchOptions) or have postCallScript call this.request with {
path: "/callScript", method: "POST", timeout: opts.timeout, fetchOptions:
opts.fetch, body: { connectedFileName: this.connectedFileName, scriptName:
opts.script, data: opts.scriptParam } }; ensure the unauthorized handling still
replays the request through the same request helper (refer to executeScript,
postCallScript, and this.request).
In `@packages/typegen/src/server/createDataApiClient.ts`:
- Around line 347-367: The FM MCP client identity is being derived from
process.cwd() which can be the wrong directory; update the code that calls
getFmMcpClientIdentity(process.cwd()) inside createDataApiClient to use the
project's root from the provided context (e.g.,
getFmMcpClientIdentity(context.projectRoot) or the appropriate context property)
so fmMcpClientIdentity is derived from the caller's project root rather than the
server's working directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8a61aef-1419-425e-906c-83d252aca7e3
📒 Files selected for processing (22)
.changeset/fm-mcp-typegen-authorization.mdapps/docs/.env.schemaapps/docs/env.d.tsapps/docs/src/app/api/proofkit/manifest/revalidate/route.tsapps/docs/tests/manifest-revalidate.test.tspackages/cli/src/core/context.tspackages/cli/src/core/resolveInitRequest.tspackages/cli/src/core/types.tspackages/cli/src/services/live.tspackages/cli/src/utils/projectFiles.tspackages/cli/tests/resolve-init.test.tspackages/cli/tests/test-layer.tspackages/fmdapi/src/adapters/fm-mcp.tspackages/fmdapi/tests/fm-mcp-adapter.test.tspackages/typegen/src/constants.tspackages/typegen/src/getEnvValues.tspackages/typegen/src/server/createDataApiClient.tspackages/typegen/src/typegen.tspackages/typegen/src/types.tspackages/typegen/tests/getEnvValues.test.tspackages/typegen/tests/typegen.test.tspackages/typegen/typegen.schema.json
Comment on lines
+623
to
+714
| it("requests FileMaker bridge authorization after unauthorized fmMcp metadata request", async () => { | ||
| process.env.FM_MCP_BASE_URL = "http://127.0.0.1:1365"; | ||
| process.env.FM_CONNECTED_FILE_NAME = "TestFile"; | ||
|
|
||
| const fetchMock = vi | ||
| .fn<typeof fetch>() | ||
| .mockResolvedValueOnce( | ||
| new Response(JSON.stringify({ code: "session_not_authorized" }), { | ||
| status: 401, | ||
| headers: { "content-type": "application/json" }, | ||
| }), | ||
| ) | ||
| .mockResolvedValueOnce( | ||
| new Response(JSON.stringify({ status: "approved" }), { | ||
| status: 200, | ||
| headers: { "content-type": "application/json" }, | ||
| }), | ||
| ) | ||
| .mockResolvedValueOnce( | ||
| new Response( | ||
| JSON.stringify({ | ||
| result: { | ||
| messages: [{ code: "0" }], | ||
| response: mockLayoutMetadata["basic-layout"], | ||
| }, | ||
| }), | ||
| { status: 200, headers: { "content-type": "application/json" } }, | ||
| ), | ||
| ); | ||
| vi.stubGlobal("fetch", fetchMock); | ||
|
|
||
| const config: Extract<z.infer<typeof typegenConfigSingle>, { type: "fmdapi" }> = { | ||
| type: "fmdapi", | ||
| envNames: undefined, | ||
| layouts: [{ layoutName: "FmMcpLayout", schemaName: "fmMcpAuthSchema" }], | ||
| path: "unit-typegen-output/fm-mcp-auth", | ||
| validator: false, | ||
| fmMcp: { | ||
| enabled: true, | ||
| sessionId: "typegen-session", | ||
| clientName: "Typegen Config Test", | ||
| clientDescription: "Typegen config authorization", | ||
| authorizationTimeoutMs: 10_000, | ||
| }, | ||
| }; | ||
|
|
||
| await generateTypedClients(config, { cwd: import.meta.dirname }); | ||
|
|
||
| expect(fetchMock).toHaveBeenCalledTimes(3); | ||
| expect(fetchMock.mock.calls[1][0]).toBe("http://127.0.0.1:1365/authorizeSession"); | ||
| const authorizeBody = JSON.parse(String(fetchMock.mock.calls[1][1]?.body ?? "{}")); | ||
| expect(authorizeBody).toMatchObject({ | ||
| sessionId: "typegen-session", | ||
| fileName: "TestFile", | ||
| clientName: "Typegen Config Test", | ||
| clientDescription: "Typegen config authorization", | ||
| }); | ||
| }); | ||
|
|
||
| it("uses fmMcp persistent token env var as session id", async () => { | ||
| process.env.FM_MCP_BASE_URL = "http://127.0.0.1:1365"; | ||
| process.env.FM_CONNECTED_FILE_NAME = "TestFile"; | ||
| process.env.TYPEGEN_PERSISTENT_TOKEN = "registered-persistent-token"; | ||
|
|
||
| const fetchMock = vi.fn( | ||
| createLayoutMetadataMock({ | ||
| FmMcpLayout: mockLayoutMetadata["basic-layout"], | ||
| }), | ||
| ); | ||
| vi.stubGlobal("fetch", fetchMock as unknown as typeof fetch); | ||
|
|
||
| const config: Extract<z.infer<typeof typegenConfigSingle>, { type: "fmdapi" }> = { | ||
| type: "fmdapi", | ||
| envNames: { | ||
| fmMcp: { | ||
| persistentToken: "TYPEGEN_PERSISTENT_TOKEN", | ||
| }, | ||
| }, | ||
| layouts: [{ layoutName: "FmMcpLayout", schemaName: "fmMcpPersistentTokenSchema" }], | ||
| path: "unit-typegen-output/fm-mcp-persistent-token", | ||
| validator: false, | ||
| fmMcp: { | ||
| enabled: true, | ||
| }, | ||
| }; | ||
|
|
||
| await generateTypedClients(config, { cwd: import.meta.dirname }); | ||
|
|
||
| const headers = fetchMock.mock.calls[0]?.[1]?.headers as Headers; | ||
| expect(headers.get("X-ProofKit-Session")).toBe("registered-persistent-token"); | ||
| expect(fetchMock).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
Restore newly-mutated env vars to avoid cross-test leakage.
Line 624 and Line 685 set env vars that aren’t tracked in originalEnv, so cleanup won’t restore them. This can make later tests order-dependent.
Suggested fix
beforeEach(async () => {
@@
- originalEnv.FM_HTTP_BASE_URL = process.env.FM_HTTP_BASE_URL;
+ originalEnv.FM_HTTP_BASE_URL = process.env.FM_HTTP_BASE_URL;
+ originalEnv.FM_MCP_BASE_URL = process.env.FM_MCP_BASE_URL;
originalEnv.FM_CONNECTED_FILE_NAME = process.env.FM_CONNECTED_FILE_NAME;
+ originalEnv.TYPEGEN_PERSISTENT_TOKEN = process.env.TYPEGEN_PERSISTENT_TOKEN;
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/typegen/src/typegen.ts (1)
238-309:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden FM MCP file auto-discovery with timeout + normalized URL.
This discovery fetch can block forever when the bridge is unreachable, which freezes typegen runs until the process is interrupted.
Proposed fix
+const fmMcpDiscoveryTimeoutMs = 15_000; ... - const res = await fetch(`${fmMcpBaseUrl}/connectedFiles`); + const normalizedBaseUrl = fmMcpBaseUrl.replace(/\/+$/, ""); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), fmMcpDiscoveryTimeoutMs); + const res = await fetch(`${normalizedBaseUrl}/connectedFiles`, { + signal: controller.signal, + }); if (res.ok) { const files = (await res.json()) as string[]; + clearTimeout(timeout); ... - } catch (_err) { + } catch (err) { + if (err instanceof Error && err.name === "AbortError") { + console.log(chalk.red("ERROR: Timed out while reaching FM MCP server.")); + return; + } console.log(chalk.red(`ERROR: Could not reach FM MCP server at ${fmMcpBaseUrl}`));🤖 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/typegen/src/typegen.ts` around lines 238 - 309, The fetch to `${fmMcpBaseUrl}/connectedFiles` can hang; update the block that uses fmMcpBaseUrl and the fetch call to first normalize fmMcpBaseUrl (trim trailing slashes or construct with new URL to avoid double slashes) and then perform the fetch using an AbortController with a configurable timeout (e.g., 5s) so the request is aborted if it takes too long; ensure the code passes the controller.signal to fetch, clears the timeout on success, and improves the catch/log path to surface timeout vs connection errors for the existing logic that handles res.ok / files.length (refer to the fmMcpBaseUrl variable and the fetch(`${fmMcpBaseUrl}/connectedFiles`) call in typegen.ts).
🧹 Nitpick comments (1)
packages/typegen/web/src/components/ConfigEditor.tsx (1)
68-73: ⚡ Quick winPreserve existing FM MCP values when toggling off.
Disabling currently sets
fmMcptoundefined, which drops previously entered bridge settings. Prefer flippingenabledwhile retaining field values.Proposed fix
const handleFmMcpToggle = (checked: boolean) => { - setValue(`config.${index}.fmMcp` as const, checked ? { enabled: true } : undefined, { + setValue( + `config.${index}.fmMcp` as const, + checked + ? { ...(fmMcp ?? {}), enabled: true } + : { ...(fmMcp ?? {}), enabled: false }, + { shouldDirty: true, shouldTouch: true, - }); + }, + ); };🤖 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/typegen/web/src/components/ConfigEditor.tsx` around lines 68 - 73, The toggle handler handleFmMcpToggle should not drop existing fmMcp fields; instead read the current value via getValues(`config.${index}.fmMcp`), build a new object that preserves other properties and only flips the enabled flag (e.g., { ...existing, enabled: checked }), and pass that to setValue (with the existing shouldDirty/shouldTouch options); when enabling and existing is undefined, initialize with enabled:true and any desired defaults but avoid setting the whole property to undefined when disabling so field values are retained.
🤖 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/typegen/src/server/createDataApiClient.ts`:
- Around line 502-509: Normalize the baseUrl used when building the FM MCP
session cache key so trailing slashes or equivalent URL-normalization
differences do not produce distinct session IDs: before constructing sessionKey
(the object with cwd, baseUrl, connectedFileName, clientName) in
createDataApiClient.ts, normalize/trim the baseUrl (e.g., remove a trailing
slash and ensure consistent scheme/host formatting) and then pass that
normalized value into getFmMcpSessionId (which computes sessionId using
persistentToken or fmMcpObj?.sessionId) so caching uses a canonical URL.
- Around line 434-485: The connected-files discovery fetch in
createDataApiClient.ts can hang; update the block that calls
fetch(`${baseUrl.replace(trailingSlashesRegex, "")}/connectedFiles`) (inside the
if (!connectedFileName) branch) to use an AbortController with a short timeout
(e.g., 5s): create an AbortController, pass controller.signal to fetch, schedule
controller.abort() with setTimeout, and clearTimeout on success; catch the abort
error and return a connection_error that clearly indicates a timeout to the
caller (preserving the existing error shape), and ensure references to baseUrl,
trailingSlashesRegex, and connectedFileNameEnvName remain unchanged.
---
Outside diff comments:
In `@packages/typegen/src/typegen.ts`:
- Around line 238-309: The fetch to `${fmMcpBaseUrl}/connectedFiles` can hang;
update the block that uses fmMcpBaseUrl and the fetch call to first normalize
fmMcpBaseUrl (trim trailing slashes or construct with new URL to avoid double
slashes) and then perform the fetch using an AbortController with a configurable
timeout (e.g., 5s) so the request is aborted if it takes too long; ensure the
code passes the controller.signal to fetch, clears the timeout on success, and
improves the catch/log path to surface timeout vs connection errors for the
existing logic that handles res.ok / files.length (refer to the fmMcpBaseUrl
variable and the fetch(`${fmMcpBaseUrl}/connectedFiles`) call in typegen.ts).
---
Nitpick comments:
In `@packages/typegen/web/src/components/ConfigEditor.tsx`:
- Around line 68-73: The toggle handler handleFmMcpToggle should not drop
existing fmMcp fields; instead read the current value via
getValues(`config.${index}.fmMcp`), build a new object that preserves other
properties and only flips the enabled flag (e.g., { ...existing, enabled:
checked }), and pass that to setValue (with the existing shouldDirty/shouldTouch
options); when enabling and existing is undefined, initialize with enabled:true
and any desired defaults but avoid setting the whole property to undefined when
disabling so field values are retained.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ad0753d-3b06-418f-a995-8013fd10000f
📒 Files selected for processing (24)
.changeset/fm-mcp-request-options.md.changeset/fm-mcp-typegen-authorization.md.changeset/short-typegen-fm-mcp-idle.md.changeset/typegen-friendly-authorization-error.md.changeset/typegen-ui-fm-mcp-memory.mdapps/docs/env.d.tspackages/cli/src/core/context.tspackages/cli/src/core/resolveInitRequest.tspackages/cli/src/services/live.tspackages/cli/tests/resolve-init.test.tspackages/cli/tests/test-layer.tspackages/fmdapi/src/adapters/fm-mcp.tspackages/fmdapi/tests/fm-mcp-adapter.test.tspackages/typegen/src/cli-errors.tspackages/typegen/src/cli.tspackages/typegen/src/fmMcpSession.tspackages/typegen/src/server/app.tspackages/typegen/src/server/createDataApiClient.tspackages/typegen/src/typegen.tspackages/typegen/tests/cli-errors.test.tspackages/typegen/tests/typegen.test.tspackages/typegen/web/src/components/ConfigEditor.tsxpackages/typegen/web/src/components/EnvVarDialog.tsxpackages/typegen/web/src/hooks/useTestConnection.ts
✅ Files skipped from review due to trivial changes (7)
- .changeset/typegen-ui-fm-mcp-memory.md
- packages/typegen/tests/cli-errors.test.ts
- .changeset/short-typegen-fm-mcp-idle.md
- .changeset/typegen-friendly-authorization-error.md
- .changeset/fm-mcp-typegen-authorization.md
- .changeset/fm-mcp-request-options.md
- apps/docs/env.d.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/fmdapi/tests/fm-mcp-adapter.test.ts
- packages/fmdapi/src/adapters/fm-mcp.ts
- packages/typegen/tests/typegen.test.ts
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
/authorizeSessiontimeout and single retry after unauthorized responses.ProofKit/ProofKit Typegenplus the project name.Testing
pnpm --filter @proofkit/typegen generate:schemapnpm --filter @proofkit/typegen typecheckpnpm --filter @proofkit/typegen test -- typegen.test.tspnpm --filter @proofkit/cli typecheckpnpm --filter @proofkit/cli test -- resolve-init.test.tspnpm run cipasses lint, typecheck, tests, and package builds; docs build still fails locally due missingPROOFKIT_MANIFEST_REVALIDATE_SECRET.Summary by CodeRabbit
New Features
Bug Fixes
Chores