Open
Conversation
- add Claris ID login flow for fmodata - wire CLI and typegen env/config support - update docs and tests
🦋 Changeset detectedLatest commit: 2f0f8f3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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: |
📝 WalkthroughWalkthroughThis pull request adds Claris ID authentication support for FileMaker Cloud connections across the fmodata and typegen packages. Changes include new AWS Cognito integration, CLI options for Claris ID credentials, updated type definitions, environment variable handling, and corresponding test coverage and web UI updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Web/CLI Client
participant Manager as ClarisIdAuthManager
participant Cognito as AWS Cognito
participant ClarisId as Claris ID Endpoint
participant FMOData as FMServerConnection
Client->>Manager: getAuthorizationHeader()
alt Token Cache Valid
Manager-->>Client: Return cached ID token header
else Token Invalid/Missing
Manager->>ClarisId: Fetch user pool config
ClarisId-->>Manager: UserPoolId, ClientId
alt Stored Session Exists
Manager->>Cognito: refreshSession(refreshToken)
Cognito-->>Manager: New CognitoUserSession
else Session Missing
Manager->>Cognito: authenticateUser(username, password)
Cognito-->>Manager: CognitoUserSession
end
Manager->>Manager: Extract ID token from session
Manager-->>Client: Return FMID authorization header
end
Client->>FMOData: HTTP request with Authorization header
FMOData-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/typegen/web/src/components/LayoutSelector.tsx (1)
25-31:⚠️ Potential issue | 🟡 MinorMissing
clarisIdPasswordproperty in themissingtype definition.The code at line 174 accesses
errorDetails.missing.clarisIdPassword, but this property is not defined in theErrorDetails.missinginterface. This will cause a TypeScript compilation error.🛠️ Proposed fix
interface ErrorDetails { missing?: { server?: boolean; db?: boolean; auth?: boolean; password?: boolean; + clarisIdPassword?: boolean; }; fmErrorCode?: string; suspectedField?: "server" | "db" | "auth"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typegen/web/src/components/LayoutSelector.tsx` around lines 25 - 31, The ErrorDetails interface is missing the clarisIdPassword property under its missing type which causes a TS error when LayoutSelector accesses errorDetails.missing.clarisIdPassword; update the ErrorDetails definition to add clarisIdPassword?: boolean to the missing shape (the interface named ErrorDetails and its nested missing object) so the property exists and matches optional usage elsewhere.packages/typegen/src/getEnvValues.ts (1)
183-207:⚠️ Potential issue | 🟡 MinorGate the auth-help text on
allowClarisId.These messages always advertise
clarisIdUsername/clarisIdPassword, but some standard callers still reject theclarisIdauth shape entirely (seepackages/typegen/src/server/createDataApiClient.ts, Lines 388-394). WhenallowClarisIdis off, the current guidance sends users toward an unsupported config.Also applies to: 312-336
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typegen/src/getEnvValues.ts` around lines 183 - 207, The missing-vars auth message currently always mentions clarisId env names; update the logic around the block using hasAnyAuth/getEnvName so that clarisIdUsernameName and clarisIdPasswordName are only computed and included in the pushed missingVars string when allowClarisId is true (i.e., gate the clarisId branches on allowClarisId when reading envNames?.auth and when building the `${apiKeyName} (or ...)` message); apply the same guard to the analogous block referenced around lines 312-336 so callers that disallow clarisId are not directed to unsupported env variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/docs/src/app/`(home)/page.tsx:
- Line 55: Update the Card component invocation that renders the product name by
correcting the title prop value from "Filemaker Data API" to "FileMaker Data
API" so the Card (the element with props href="/docs/fmdapi", icon={<WebhookIcon
/>}, title=...) uses the proper branding; locate the Card call in page.tsx and
change only the title string.
In `@apps/docs/src/app/docs/`(docs)/layout.tsx:
- Line 19: Change the anchor with className "underline" that links to
"http://proof.sh" so it uses HTTPS and includes noreferrer: update the href to
"https://proof.sh" and set rel to "noopener noreferrer" (preserving
target="_blank") to match the other template and ensure both security and
privacy.
In `@apps/docs/src/app/docs/templates/layout.tsx`:
- Line 89: Update the external anchor in the docs layout that currently uses
href="http://proof.sh" (the <a className="underline" ...> in layout.tsx) to use
the secure protocol by changing the href to "https://proof.sh" while leaving
rel="noopener noreferrer" and target="_blank" intact.
In `@packages/fmodata/src/cli/utils/connection.ts`:
- Around line 32-36: The current locals (apiKey, username, password,
clarisIdUsername, clarisIdPassword) mix CLI flags and env vars per-field which
lets leftover env vars override an explicitly selected CLI auth mode; instead,
determine the auth mode from CLI flags first (inspect opts.apiKey,
opts.clarisIdUsername/opts.clarisIdPassword, opts.username/opts.password) and
only if none of those CLI flags were provided fall back to reading ENV_NAMES.*;
then populate the same locals (apiKey, clarisIdUsername, clarisIdPassword,
username, password) from envs only as a fallback. Apply this change consistently
for the other block referenced (lines 56–70) so auth selection precedence is:
explicit CLI flags -> env vars.
In `@packages/fmodata/src/client/filemaker-odata.ts`:
- Around line 256-261: The code currently uses “const fetchHandler =
options?.fetchHandler ?? this._fetchClientOptions?.fetchHandler” and then
recreates a client whenever fetchHandler is truthy, which replaces
constructor-level fetch client options; change the logic to only create a
temporary client when a per-request override exists (i.e., when
options?.fetchHandler is provided), otherwise use this.fetchClient; update the
client selection expression from "fetchHandler ? createClient({ retries: 0,
fetchHandler }) : this.fetchClient" to check options?.fetchHandler (or
explicitly compare against undefined) so constructor-configured
this._fetchClientOptions and this.fetchClient are preserved, and keep using
fetchHandler (from options) when creating the temporary client via createClient.
- Around line 246-252: buildHeaders is spreading options?.headers which breaks
when callers pass a Headers instance or Header tuples; change buildHeaders to
create a new Headers() instance, set Authorization, Content-Type, Accept and
Prefer via headers.set or headers.append, then if options?.headers is present
iterate its entries (supporting Headers, [string,string][], or Record) and set
each into the Headers instance; also update the later place that destructures
the returned headers (the code that assumes a plain object) to either read from
the Headers via headers.get(...) or convert the Headers to a plain object by
iterating headers.entries(), so no code assumes headers is a plain object.
In `@packages/fmodata/tests/cli/unit/connection.test.ts`:
- Around line 120-130: The test currently only checks that db is defined, which
is true regardless of which auth branch runs; update the test that uses
buildConnection to make the username auth intentionally incomplete (e.g.,
provide username but omit or set password to undefined/empty) while keeping
clarisIdUsername and clarisIdPassword present so the only valid credential set
is the Claris ID one; this ensures buildConnection's preference for Claris ID is
actually validated (refer to buildConnection and the test block name "prefers
Claris ID over username auth when both are set").
In `@packages/typegen/src/getEnvValues.ts`:
- Around line 152-153: The code treats a partial Claris ID (e.g.
clarisIdUsername only) as valid; change the boolean logic so hasClarisIdAuth is
true only when options?.allowClarisId is true AND both clarisIdUsername and
clarisIdPassword are present, and ensure validateAndLogEnvValues uses these same
booleans (hasClarisIdAuth and hasAnyAuth) rather than raw env vars; update any
other similar checks that infer Claris ID availability (e.g., places referencing
clarisIdUsername/clarisIdPassword directly) to use hasClarisIdAuth so auth
selection/logging prefers a truly complete apiKey or username/password before
choosing clarisId.
In `@packages/typegen/src/server/createDataApiClient.ts`:
- Around line 388-395: The branch checking "clarisId" on auth is unreachable
because createClientFromConfig calls getEnvVarsFromConfig without allowClarisId,
so auth will never include clarisId; fix by making createClientFromConfig
request CLARIS_ID variables when evaluating an fmdapi config: call
getEnvVarsFromConfig(config.envNames, { allowClarisId: true }) (or otherwise
propagate allowClarisId=true) before the existing "clarisId" check so the auth
object can contain clarisId and the adapter-specific error branch in
createDataApiClient (the "clarisId" in auth branch) can be hit.
In `@packages/typegen/tests/odata-client-config.test.ts`:
- Line 40: The test sets process.env.CLARIS_ID_PASSWORD using an assignment
which stores the string "undefined" and prevents the missing-password branch
from running; change the test to remove the env var (use delete
process.env.CLARIS_ID_PASSWORD) so the validation that checks !clarisIdPassword
actually sees the variable as absent and triggers the missing-password error
path; update the line that currently assigns process.env.CLARIS_ID_PASSWORD to
instead delete that property.
In `@packages/typegen/web/src/components/EnvVarDialog.tsx`:
- Around line 107-120: The dialog currently sets activeAuthMethod to "clarisId"
only when both clarisIdUsernameValue and clarisIdPasswordValue are non-empty;
change the logic so precedence matches the server: if apiKeyValue is present set
activeAuthMethod = "apiKey", else if clarisIdUsernameValue is present
(non-empty) set activeAuthMethod = "clarisId" even if clarisIdPasswordValue is
missing, otherwise fall back to "username" or null; update both occurrences that
assign activeAuthMethod (the block using activeAuthMethod and the similar block
around lines 145-174) so the UI highlights Claris ID as soon as
clarisIdUsernameValue exists and allows the server to surface the
missing-password error.
---
Outside diff comments:
In `@packages/typegen/src/getEnvValues.ts`:
- Around line 183-207: The missing-vars auth message currently always mentions
clarisId env names; update the logic around the block using
hasAnyAuth/getEnvName so that clarisIdUsernameName and clarisIdPasswordName are
only computed and included in the pushed missingVars string when allowClarisId
is true (i.e., gate the clarisId branches on allowClarisId when reading
envNames?.auth and when building the `${apiKeyName} (or ...)` message); apply
the same guard to the analogous block referenced around lines 312-336 so callers
that disallow clarisId are not directed to unsupported env variables.
In `@packages/typegen/web/src/components/LayoutSelector.tsx`:
- Around line 25-31: The ErrorDetails interface is missing the clarisIdPassword
property under its missing type which causes a TS error when LayoutSelector
accesses errorDetails.missing.clarisIdPassword; update the ErrorDetails
definition to add clarisIdPassword?: boolean to the missing shape (the interface
named ErrorDetails and its nested missing object) so the property exists and
matches optional usage elsewhere.
🪄 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: e8e63bde-b277-4a89-8d39-d74c5518ed61
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.changeset/early-eels-smile.mdapps/docs/src/app/(home)/page.tsxapps/docs/src/app/docs/(docs)/layout.tsxapps/docs/src/app/docs/templates/layout.tsxpackages/fmodata/README.mdpackages/fmodata/package.jsonpackages/fmodata/skills/fmodata-client/SKILL.mdpackages/fmodata/src/cli/index.tspackages/fmodata/src/cli/utils/connection.tspackages/fmodata/src/client/amazon-cognito-identity-js.d.tspackages/fmodata/src/client/claris-id.tspackages/fmodata/src/client/filemaker-odata.tspackages/fmodata/src/types.tspackages/fmodata/tests/claris-id.test.tspackages/fmodata/tests/cli/unit/connection.test.tspackages/typegen/README.mdpackages/typegen/src/constants.tspackages/typegen/src/fmodata/downloadMetadata.tspackages/typegen/src/getEnvValues.tspackages/typegen/src/server/app.tspackages/typegen/src/server/createDataApiClient.tspackages/typegen/src/typegen.tspackages/typegen/src/types.tspackages/typegen/tests/getEnvValues.test.tspackages/typegen/tests/odata-client-config.test.tspackages/typegen/vitest.config.tspackages/typegen/web/src/components/EnvVarDialog.tsxpackages/typegen/web/src/components/LayoutSelector.tsxpackages/typegen/web/src/components/useEnvVarIndicator.tspackages/typegen/web/src/hooks/useTestConnection.ts
| > | ||
| A type-safe API for your FileMaker layouts. Easily connect without | ||
| worrying about token management. | ||
| <Card href="/docs/fmdapi" icon={<WebhookIcon />} title="Filemaker Data API"> |
There was a problem hiding this comment.
Fix product naming capitalization in the card title.
At Line 55, "Filemaker Data API" should be "FileMaker Data API" for consistent branding across the page.
✏️ Suggested text fix
- <Card href="/docs/fmdapi" icon={<WebhookIcon />} title="Filemaker Data API">
+ <Card href="/docs/fmdapi" icon={<WebhookIcon />} title="FileMaker Data API">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Card href="/docs/fmdapi" icon={<WebhookIcon />} title="Filemaker Data API"> | |
| <Card href="/docs/fmdapi" icon={<WebhookIcon />} title="FileMaker Data API"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/docs/src/app/`(home)/page.tsx at line 55, Update the Card component
invocation that renders the product name by correcting the title prop value from
"Filemaker Data API" to "FileMaker Data API" so the Card (the element with props
href="/docs/fmdapi", icon={<WebhookIcon />}, title=...) uses the proper
branding; locate the Card call in page.tsx and change only the title string.
| rel="noopener" | ||
| target="_blank" | ||
| > | ||
| <a className="underline" href="http://proof.sh" rel="noopener" target="_blank"> |
There was a problem hiding this comment.
Use HTTPS and add noreferrer for consistency and security.
The external link to proof.sh should use HTTPS instead of HTTP. Additionally, for consistency with the same link in apps/docs/src/app/docs/templates/layout.tsx (line 89), consider adding noreferrer to the rel attribute. Using rel="noopener noreferrer" provides both security (noopener prevents access to window.opener) and privacy (noreferrer prevents sending the referrer header).
🔒 Suggested fix
- <a className="underline" href="http://proof.sh" rel="noopener" target="_blank">
+ <a className="underline" href="https://proof.sh" rel="noopener noreferrer" target="_blank">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <a className="underline" href="http://proof.sh" rel="noopener" target="_blank"> | |
| <a className="underline" href="https://proof.sh" rel="noopener noreferrer" target="_blank"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/docs/src/app/docs/`(docs)/layout.tsx at line 19, Change the anchor with
className "underline" that links to "http://proof.sh" so it uses HTTPS and
includes noreferrer: update the href to "https://proof.sh" and set rel to
"noopener noreferrer" (preserving target="_blank") to match the other template
and ensure both security and privacy.
| rel="noopener noreferrer" | ||
| target="_blank" | ||
| > | ||
| <a className="underline" href="http://proof.sh" rel="noopener noreferrer" target="_blank"> |
There was a problem hiding this comment.
Use HTTPS for the external link.
The external link to proof.sh should use HTTPS instead of HTTP for security. While the rel="noopener noreferrer" attribute is correctly set, the protocol should be secure.
🔒 Suggested fix
- <a className="underline" href="http://proof.sh" rel="noopener noreferrer" target="_blank">
+ <a className="underline" href="https://proof.sh" rel="noopener noreferrer" target="_blank">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <a className="underline" href="http://proof.sh" rel="noopener noreferrer" target="_blank"> | |
| <a className="underline" href="https://proof.sh" rel="noopener noreferrer" target="_blank"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/docs/src/app/docs/templates/layout.tsx` at line 89, Update the external
anchor in the docs layout that currently uses href="http://proof.sh" (the <a
className="underline" ...> in layout.tsx) to use the secure protocol by changing
the href to "https://proof.sh" while leaving rel="noopener noreferrer" and
target="_blank" intact.
Comment on lines
32
to
+36
| const apiKey = opts.apiKey ?? process.env[ENV_NAMES.apiKey]; | ||
| const username = opts.username ?? process.env[ENV_NAMES.username]; | ||
| const password = opts.password ?? process.env[ENV_NAMES.password]; | ||
| const clarisIdUsername = opts.clarisIdUsername ?? process.env[ENV_NAMES.clarisIdUsername]; | ||
| const clarisIdPassword = opts.clarisIdPassword ?? process.env[ENV_NAMES.clarisIdPassword]; |
There was a problem hiding this comment.
Resolve auth mode from explicit CLI flags before falling back to env vars.
These locals mix sources per field and then apply apiKey > clarisId > username precedence. A leftover OTTO_API_KEY or CLARIS_ID_* env var can therefore silently override an explicit --claris-id-* or --username/--password selection, which makes the new Claris ID flow unreliable from the CLI.
Also applies to: 56-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/fmodata/src/cli/utils/connection.ts` around lines 32 - 36, The
current locals (apiKey, username, password, clarisIdUsername, clarisIdPassword)
mix CLI flags and env vars per-field which lets leftover env vars override an
explicitly selected CLI auth mode; instead, determine the auth mode from CLI
flags first (inspect opts.apiKey, opts.clarisIdUsername/opts.clarisIdPassword,
opts.username/opts.password) and only if none of those CLI flags were provided
fall back to reading ENV_NAMES.*; then populate the same locals (apiKey,
clarisIdUsername, clarisIdPassword, username, password) from envs only as a
fallback. Apply this change consistently for the other block referenced (lines
56–70) so auth selection precedence is: explicit CLI flags -> env vars.
Comment on lines
+246
to
+252
| const buildHeaders = async () => ({ | ||
| Authorization: await this._getAuthorizationHeader(fetchHandler), | ||
| "Content-Type": "application/json", | ||
| Accept: getAcceptHeader(includeODataAnnotations), | ||
| ...(preferValues.length > 0 ? { Prefer: preferValues.join(", ") } : {}), | ||
| ...(options?.headers || {}), | ||
| }; | ||
|
|
||
| // Prepare loggableHeaders by omitting the Authorization key | ||
| const { Authorization, ...loggableHeaders } = headers; | ||
| logger.debug("Request headers:", loggableHeaders); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the Fetch API's RequestInit.headersacceptHeaders, header tuple arrays, and plain objects, and what happens if you spread a Headers object or tuple array into an object literal instead of iterating its entries?
💡 Result:
Yes.
What RequestInit.headers accepts
RequestInit.headers is a HeadersInit-style value, which is commonly (and in TypeScript) treated as:
- a
Headersinstance - an array of
[name, value]pairs (string[][]) - a plain object / record (
Record<string, string>)
MDN documents that the headers option can be a Headers object or an object literal. [1] The Fetch Standard defines HeadersInit as either a sequence of 2-item sequences (tuple/pair arrays) or a record/object. [2] And new Headers(init) explicitly accepts all three forms (object, tuple array, or existing Headers). [3]
What happens if you spread them into an object literal
Object spread ({ ...x }) copies only own, enumerable properties of x—it does not use an iterable’s entries. [4][5]
So:
- Spreading a
Headersinstance into an object literal gives{}(because header fields aren’t exposed as own enumerable properties; you must iterate entries instead). - Spreading a tuple array into an object literal gives numeric keys (array indices), e.g.
{0: ["accept","*/*"], 1: ["x-foo","bar"]}, not{accept:"*/*", "x-foo":"bar"}—because arrays’ enumerable properties are their index properties. [4][6]
Use this instead
Object.fromEntries(headers.entries())orObject.fromEntries(headers)Object.fromEntries(tupleArray)
Sources: [1] [2] [3] [4] [5] [6]
🏁 Script executed:
cat -n packages/fmodata/src/client/filemaker-odata.ts | sed -n '240,275p'Repository: proofsh/proofkit
Length of output: 1819
🏁 Script executed:
rg -A 5 -B 5 "RequestInit|HeadersInit" packages/fmodata/src/client/filemaker-odata.ts | head -30Repository: proofsh/proofkit
Length of output: 916
🏁 Script executed:
rg -B 10 "const buildHeaders" packages/fmodata/src/client/filemaker-odata.ts | head -20Repository: proofsh/proofkit
Length of output: 400
HeadersInit corruption when spreading non-plain-object headers.
RequestInit.headers accepts Headers instances, tuple arrays, or plain objects. Spreading options?.headers at line 251 loses all entries when it's a Headers instance (spreads to {}) and produces numeric keys when it's a tuple array, so valid caller-supplied headers disappear or become invalid. Additionally, line 267's destructuring assumes headers is a plain object and will fail if buildHeaders() returns a Headers instance.
Use the Headers constructor and iterate entries instead:
🛠️ Suggested fix
- const buildHeaders = async () => ({
- Authorization: await this._getAuthorizationHeader(fetchHandler),
- "Content-Type": "application/json",
- Accept: getAcceptHeader(includeODataAnnotations),
- ...(preferValues.length > 0 ? { Prefer: preferValues.join(", ") } : {}),
- ...(options?.headers || {}),
- });
+ const buildHeaders = async (): Promise<Headers> => {
+ const headers = new Headers({
+ Authorization: await this._getAuthorizationHeader(fetchHandler),
+ "Content-Type": "application/json",
+ Accept: getAcceptHeader(includeODataAnnotations),
+ ...(preferValues.length > 0 ? { Prefer: preferValues.join(", ") } : {}),
+ });
+ for (const [name, value] of new Headers(options?.headers).entries()) {
+ headers.set(name, value);
+ }
+ return headers;
+ };
...
- const { Authorization, ...loggableHeaders } = headers;
+ const loggableHeaders = Object.fromEntries(headers.entries());
+ delete loggableHeaders.authorization;Also applies to: 266-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/fmodata/src/client/filemaker-odata.ts` around lines 246 - 252,
buildHeaders is spreading options?.headers which breaks when callers pass a
Headers instance or Header tuples; change buildHeaders to create a new Headers()
instance, set Authorization, Content-Type, Accept and Prefer via headers.set or
headers.append, then if options?.headers is present iterate its entries
(supporting Headers, [string,string][], or Record) and set each into the Headers
instance; also update the later place that destructures the returned headers
(the code that assumes a plain object) to either read from the Headers via
headers.get(...) or convert the Headers to a plain object by iterating
headers.entries(), so no code assumes headers is a plain object.
Comment on lines
+120
to
+130
| it("prefers Claris ID over username auth when both are set", () => { | ||
| const { db } = buildConnection({ | ||
| server: "https://example.com", | ||
| database: "MyDB.fmp12", | ||
| clarisIdUsername: "claris-user", | ||
| clarisIdPassword: "claris-pass", | ||
| username: "admin", | ||
| password: "secret", | ||
| }); | ||
| expect(db).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Precedence test does not currently validate precedence.
db being defined would pass for either auth branch. Make username auth incomplete so success proves Claris ID was chosen.
Suggested test adjustment
it("prefers Claris ID over username auth when both are set", () => {
const { db } = buildConnection({
server: "https://example.com",
database: "MyDB.fmp12",
clarisIdUsername: "claris-user",
clarisIdPassword: "claris-pass",
username: "admin",
- password: "secret",
+ // intentionally omit password so username auth would fail if selected
});
expect(db).toBeDefined();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("prefers Claris ID over username auth when both are set", () => { | |
| const { db } = buildConnection({ | |
| server: "https://example.com", | |
| database: "MyDB.fmp12", | |
| clarisIdUsername: "claris-user", | |
| clarisIdPassword: "claris-pass", | |
| username: "admin", | |
| password: "secret", | |
| }); | |
| expect(db).toBeDefined(); | |
| }); | |
| it("prefers Claris ID over username auth when both are set", () => { | |
| const { db } = buildConnection({ | |
| server: "https://example.com", | |
| database: "MyDB.fmp12", | |
| clarisIdUsername: "claris-user", | |
| clarisIdPassword: "claris-pass", | |
| username: "admin", | |
| // intentionally omit password so username auth would fail if selected | |
| }); | |
| expect(db).toBeDefined(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/fmodata/tests/cli/unit/connection.test.ts` around lines 120 - 130,
The test currently only checks that db is defined, which is true regardless of
which auth branch runs; update the test that uses buildConnection to make the
username auth intentionally incomplete (e.g., provide username but omit or set
password to undefined/empty) while keeping clarisIdUsername and clarisIdPassword
present so the only valid credential set is the Claris ID one; this ensures
buildConnection's preference for Claris ID is actually validated (refer to
buildConnection and the test block name "prefers Claris ID over username auth
when both are set").
Comment on lines
+152
to
+153
| const hasClarisIdAuth = !!(options?.allowClarisId && clarisIdUsername); | ||
| const hasAnyAuth = !!(apiKey || hasClarisIdAuth || username); |
There was a problem hiding this comment.
Don't let partial Claris ID env vars poison a valid auth configuration.
With allowClarisId: true, clarisIdUsername alone currently makes the Claris path look selected. That later trips the missing-password branch — and drives auth selection/logging toward clarisId — even when apiKey or a complete username/password pair is already usable.
Suggested direction
- const hasClarisIdAuth = !!(options?.allowClarisId && clarisIdUsername);
- const hasAnyAuth = !!(apiKey || hasClarisIdAuth || username);
+ const hasUsernameAuth = !!(username && password);
+ const hasClarisIdCandidate = !!(options?.allowClarisId && clarisIdUsername);
+ const hasClarisIdAuth = !!(hasClarisIdCandidate && clarisIdPassword);
+ const hasAnyAuth = !!(apiKey || hasClarisIdCandidate || username);
...
- if (options?.allowClarisId && clarisIdUsername && !clarisIdPassword) {
+ if (!apiKey && !hasUsernameAuth && hasClarisIdCandidate && !clarisIdPassword) {
const clarisIdPasswordName = getEnvName(
envNames?.auth && "clarisIdPassword" in envNames.auth ? envNames.auth.clarisIdPassword : undefined,
defaultEnvNames.clarisIdPassword,
);
return {
success: false,
errorMessage: `Password is required when using Claris ID authentication. Missing: ${clarisIdPasswordName}`,
};
}Use the same booleans in validateAndLogEnvValues() so the CLI output stays aligned.
Also applies to: 216-226, 243-249, 301-303, 337-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/typegen/src/getEnvValues.ts` around lines 152 - 153, The code treats
a partial Claris ID (e.g. clarisIdUsername only) as valid; change the boolean
logic so hasClarisIdAuth is true only when options?.allowClarisId is true AND
both clarisIdUsername and clarisIdPassword are present, and ensure
validateAndLogEnvValues uses these same booleans (hasClarisIdAuth and
hasAnyAuth) rather than raw env vars; update any other similar checks that infer
Claris ID availability (e.g., places referencing
clarisIdUsername/clarisIdPassword directly) to use hasClarisIdAuth so auth
selection/logging prefers a truly complete apiKey or username/password before
choosing clarisId.
Comment on lines
+388
to
+395
| if ("clarisId" in auth) { | ||
| return { | ||
| error: "Claris ID auth is not supported for fmdapi adapters", | ||
| statusCode: 400, | ||
| kind: "adapter_error", | ||
| suspectedField: "auth", | ||
| message: "Claris ID auth is not supported for fmdapi adapters", | ||
| }; |
There was a problem hiding this comment.
This Claris ID rejection path is unreachable as written.
createClientFromConfig() still gets its auth from getEnvVarsFromConfig(config.envNames) without allowClarisId, so auth can never contain clarisId here. A fmdapi config that only has CLARIS_ID_* set will therefore fall back to the generic missing-auth error instead of the adapter-specific error this branch is trying to return.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/typegen/src/server/createDataApiClient.ts` around lines 388 - 395,
The branch checking "clarisId" on auth is unreachable because
createClientFromConfig calls getEnvVarsFromConfig without allowClarisId, so auth
will never include clarisId; fix by making createClientFromConfig request
CLARIS_ID variables when evaluating an fmdapi config: call
getEnvVarsFromConfig(config.envNames, { allowClarisId: true }) (or otherwise
propagate allowClarisId=true) before the existing "clarisId" check so the auth
object can contain clarisId and the adapter-specific error branch in
createDataApiClient (the "clarisId" in auth branch) can be hit.
| process.env.FM_SERVER = "https://example.com"; | ||
| process.env.FM_DATABASE = "MyDB.fmp12"; | ||
| process.env.CLARIS_ID_USERNAME = "claris-user"; | ||
| process.env.CLARIS_ID_PASSWORD = undefined; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify any process.env assignments to undefined that can mask "missing env" tests
rg -nP --type=ts 'process\.env\.[A-Za-z0-9_]+\s*=\s*undefined\b'Repository: proofsh/proofkit
Length of output: 161
🏁 Script executed:
# Read the test file to understand the context around line 40
cd packages/typegen/tests && head -60 odata-client-config.test.ts | tail -30Repository: proofsh/proofkit
Length of output: 845
🏁 Script executed:
# Also get more context from the beginning to understand the test setup
cd packages/typegen/tests && head -50 odata-client-config.test.tsRepository: proofsh/proofkit
Length of output: 1527
🏁 Script executed:
# Check how the password is actually checked in the code
rg -A 5 -B 5 "CLARIS_ID_PASSWORD" packages/typegen/tests/odata-client-config.test.tsRepository: proofsh/proofkit
Length of output: 860
🏁 Script executed:
# Find the implementation that checks CLARIS_ID_PASSWORD
find packages -name "*.ts" -not -path "*/node_modules/*" -not -path "*/tests/*" | xargs rg -l "CLARIS_ID_PASSWORD" | head -5Repository: proofsh/proofkit
Length of output: 138
🏁 Script executed:
# Check the implementation that validates the password
rg -B 5 -A 5 "Claris ID password" packages --type ts --type jsRepository: proofsh/proofkit
Length of output: 3486
🏁 Script executed:
# Let me test Node.js process.env behavior to confirm the actual issue
node << 'EOF'
// Test 1: Direct assignment to undefined on a plain object (like the test does)
const testEnv = { CLARIS_ID_PASSWORD: "original" };
testEnv.CLARIS_ID_PASSWORD = undefined;
console.log("Plain object assignment to undefined:");
console.log("Value:", testEnv.CLARIS_ID_PASSWORD);
console.log("Type:", typeof testEnv.CLARIS_ID_PASSWORD);
console.log("Strict equality to undefined:", testEnv.CLARIS_ID_PASSWORD === undefined);
console.log("Truthiness:", Boolean(testEnv.CLARIS_ID_PASSWORD));
// Test 2: What happens with the real process.env
console.log("\nReal process.env behavior:");
process.env.TEST_UNDEFINED = undefined;
console.log("After setting to undefined, value:", process.env.TEST_UNDEFINED);
console.log("Type:", typeof process.env.TEST_UNDEFINED);
EOFRepository: proofsh/proofkit
Length of output: 269
🏁 Script executed:
# Get the full context of how password validation works
rg -B 10 -A 5 "username && !password" packages/typegen/src/server/createDataApiClient.tsRepository: proofsh/proofkit
Length of output: 865
🏁 Script executed:
# Check the getEnvValues function to understand how it retrieves the password
rg -B 5 -A 10 "CLARIS_ID_PASSWORD" packages/typegen/src --type ts | head -60Repository: proofsh/proofkit
Length of output: 885
🏁 Script executed:
# Find where clarisIdPassword is read from process.env
rg -B 5 -A 5 "process.env.*clarisIdPassword|getEnvValues" packages/typegen/src/server/createDataApiClient.ts | head -80Repository: proofsh/proofkit
Length of output: 575
🏁 Script executed:
# Check the getEnvValues implementation
cat packages/typegen/src/getEnvValues.tsRepository: proofsh/proofkit
Length of output: 11672
🏁 Script executed:
# Let's verify the exact Node.js behavior with process.env and the string "undefined"
node << 'EOF'
// Test what actually happens with process.env and undefined
process.env.TEST_UNDEFINED = undefined;
console.log("Value:", process.env.TEST_UNDEFINED);
console.log("Type:", typeof process.env.TEST_UNDEFINED);
console.log("Strict equality to undefined:", process.env.TEST_UNDEFINED === undefined);
console.log("Strict equality to string 'undefined':", process.env.TEST_UNDEFINED === "undefined");
console.log("Boolean check (!value):", !process.env.TEST_UNDEFINED);
console.log("");
// Compare with delete
delete process.env.TEST_DELETE;
process.env.TEST_DELETE = "value";
delete process.env.TEST_DELETE;
console.log("After delete:");
console.log("Value:", process.env.TEST_DELETE);
console.log("Type:", typeof process.env.TEST_DELETE);
console.log("Strict equality to undefined:", process.env.TEST_DELETE === undefined);
console.log("Boolean check (!value):", !process.env.TEST_DELETE);
EOFRepository: proofsh/proofkit
Length of output: 311
process.env assignment to undefined coerces to the string "undefined", preventing the missing-password error path from being tested.
When process.env.CLARIS_ID_PASSWORD = undefined is executed, the value becomes the string "undefined" (not JavaScript's undefined). The validation check !clarisIdPassword then evaluates to false because a non-empty string is truthy, so the test never exercises the intended missing-password error scenario. Use delete instead to properly remove the key.
Proposed fix
- process.env.CLARIS_ID_PASSWORD = undefined;
+ delete process.env.CLARIS_ID_PASSWORD;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| process.env.CLARIS_ID_PASSWORD = undefined; | |
| delete process.env.CLARIS_ID_PASSWORD; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/typegen/tests/odata-client-config.test.ts` at line 40, The test sets
process.env.CLARIS_ID_PASSWORD using an assignment which stores the string
"undefined" and prevents the missing-password branch from running; change the
test to remove the env var (use delete process.env.CLARIS_ID_PASSWORD) so the
validation that checks !clarisIdPassword actually sees the variable as absent
and triggers the missing-password error path; update the line that currently
assigns process.env.CLARIS_ID_PASSWORD to instead delete that property.
Comment on lines
+107
to
+120
| // Precedence matches server-side auth resolution | ||
| let activeAuthMethod: "apiKey" | "clarisId" | "username" | null = null; | ||
| if (!apiKeyLoading && apiKeyValue !== undefined && apiKeyValue !== null && apiKeyValue !== "") { | ||
| activeAuthMethod = "apiKey"; | ||
| } else if ( | ||
| !(clarisIdUsernameLoading || clarisIdPasswordLoading) && | ||
| clarisIdUsernameValue !== undefined && | ||
| clarisIdUsernameValue !== null && | ||
| clarisIdUsernameValue !== "" && | ||
| clarisIdPasswordValue !== undefined && | ||
| clarisIdPasswordValue !== null && | ||
| clarisIdPasswordValue !== "" | ||
| ) { | ||
| activeAuthMethod = "clarisId"; |
There was a problem hiding this comment.
Match the dialog's auth selection to the server's precedence rules.
The server treats Claris ID as the chosen auth mode as soon as clarisIdUsername exists and then errors if clarisIdPassword is missing (packages/typegen/src/server/createDataApiClient.ts, Lines 130-197). Here the dialog only switches to "clarisId" when both fields are populated, so CLARIS_ID_USERNAME plus valid username/password makes the UI highlight Username/Password and clear the warning badge even though the connection test will fail on missing Claris ID password.
🛠️ Suggested fix
- let activeAuthMethod: "apiKey" | "clarisId" | "username" | null = null;
- if (!apiKeyLoading && apiKeyValue !== undefined && apiKeyValue !== null && apiKeyValue !== "") {
- activeAuthMethod = "apiKey";
- } else if (
- !(clarisIdUsernameLoading || clarisIdPasswordLoading) &&
- clarisIdUsernameValue !== undefined &&
- clarisIdUsernameValue !== null &&
- clarisIdUsernameValue !== "" &&
- clarisIdPasswordValue !== undefined &&
- clarisIdPasswordValue !== null &&
- clarisIdPasswordValue !== ""
- ) {
- activeAuthMethod = "clarisId";
- } else if (
- !(usernameLoading || passwordLoading) &&
- usernameValue !== undefined &&
- usernameValue !== null &&
- usernameValue !== "" &&
- passwordValue !== undefined &&
- passwordValue !== null &&
- passwordValue !== ""
- ) {
- activeAuthMethod = "username";
- }
+ const hasApiKeyAuth = !apiKeyLoading && apiKeyValue != null && apiKeyValue !== "";
+ const hasClarisIdUsername =
+ !clarisIdUsernameLoading && clarisIdUsernameValue != null && clarisIdUsernameValue !== "";
+ const hasClarisIdPassword =
+ !clarisIdPasswordLoading && clarisIdPasswordValue != null && clarisIdPasswordValue !== "";
+
+ const activeAuthMethod = hasApiKeyAuth
+ ? "apiKey"
+ : hasClarisIdUsername
+ ? "clarisId"
+ : hasUsernamePasswordAuth
+ ? "username"
+ : null;
...
- const hasAuth = hasApiKeyAuth || hasClarisIdAuth || hasUsernamePasswordAuth;
+ const hasAuth = hasApiKeyAuth || (hasClarisIdUsername ? hasClarisIdPassword : hasUsernamePasswordAuth);Also applies to: 145-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/typegen/web/src/components/EnvVarDialog.tsx` around lines 107 - 120,
The dialog currently sets activeAuthMethod to "clarisId" only when both
clarisIdUsernameValue and clarisIdPasswordValue are non-empty; change the logic
so precedence matches the server: if apiKeyValue is present set activeAuthMethod
= "apiKey", else if clarisIdUsernameValue is present (non-empty) set
activeAuthMethod = "clarisId" even if clarisIdPasswordValue is missing,
otherwise fall back to "username" or null; update both occurrences that assign
activeAuthMethod (the block using activeAuthMethod and the similar block around
lines 145-174) so the UI highlights Claris ID as soon as clarisIdUsernameValue
exists and allows the server to surface the missing-password error.
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
@proofkit/fmodatafor FileMaker Cloud.Testing
pnpm run cinot run.FMIDauth header propagation.Summary by CodeRabbit
New Features
--claris-id-usernameand--claris-id-passwordCLI options with corresponding environment variable support.Documentation
Style