Skip to content

feat(ai): add multi-provider AI support with enhanced extraction#547

Merged
kelvinkipruto merged 7 commits intomainfrom
ft/mutliprovider-ai-llms
Mar 6, 2026
Merged

feat(ai): add multi-provider AI support with enhanced extraction#547
kelvinkipruto merged 7 commits intomainfrom
ft/mutliprovider-ai-llms

Conversation

@kelvinkipruto
Copy link
Copy Markdown
Contributor

@kelvinkipruto kelvinkipruto commented Feb 24, 2026

Description

  • Introduce provider catalogue and registry to support multiple AI providers (Google, OpenAI, Anthropic, etc.)
  • Add provider credentials management in settings with environment variable fallbacks
  • Implement two-pass extraction with chunking and tool-based normalisation
  • Add force re-extract capability for existing documents
  • Update dependencies to the latest AI SDK versions and Zod 4.3.6
  • Deprecate legacy Google-only model configuration in favour of the new provider system

Fixes #538

Type of change

  • New feature (non-breaking change which adds functionality)

Screenshots

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

- Introduce provider catalog and registry to support multiple AI providers (Google, OpenAI, Anthropic, etc.)
- Add provider credentials management in settings with environment variable fallbacks
- Implement two-pass extraction with chunking and tool-based normalization
- Add force re-extract capability for existing documents
- Update dependencies to latest AI SDK versions and Zod 4.3.6
- Deprecate legacy Google-only model configuration in favor of new provider system
@vercel
Copy link
Copy Markdown

vercel Bot commented Feb 24, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
PromiseTracker Ready Ready Preview, Comment Mar 3, 2026 0:31am

Request Review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eb9861ef8d

ℹ️ 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".

Comment thread src/tasks/extractPromises.ts Outdated
Comment thread src/tasks/extractPromises.ts
@kelvinkipruto kelvinkipruto marked this pull request as draft February 24, 2026 07:32
@kelvinkipruto kelvinkipruto self-assigned this Feb 24, 2026
@kelvinkipruto kelvinkipruto marked this pull request as ready for review February 24, 2026 07:32
@kelvinkipruto
Copy link
Copy Markdown
Contributor Author

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented Feb 24, 2026

Claude finished @kelvinkipruto's task in 2m 19s —— View job


Code Review

  • Gathering context and reading changed files
  • Reviewing provider catalog and registry
  • Reviewing extraction logic changes
  • Reviewing settings changes
  • Compiling review feedback

Overall this is a well-structured feature. The provider catalog/registry split is clean, backward compatibility with legacy config is handled gracefully, and the two-pass extraction with chunking is a meaningful improvement. Below are issues ranging from critical to minor.

P1 — Bugs / Data Loss

1. Data deleted before new extraction succeeds (src/tasks/extractPromises.ts:522-528)

When forceReextract is true, all existing extractions are deleted before the AI run begins. If any subsequent step fails (empty text, API outage, parsing error), the catch block marks the document failed — but the previous extraction data is permanently gone.

// Existing extractions deleted here...
for (const extraction of existingExtractions) {
  await payload.delete({ collection: "ai-extractions", id: extraction.id });
}

// ...but extraction can still fail after this point
const extractedCandidates = await extractPromisesFromChunks({...});

Fix: Defer deletion until after the new ai-extractions record is successfully created, or use a transaction / rollback pattern. Fix this →


2. All-chunk failures produce false negatives (src/tasks/extractPromises.ts:286-294)

APICallError and NoObjectGeneratedError are caught per-chunk and swallowed with continue. If every chunk fails (e.g. due to a provider outage or unsupported structured output), candidates will be empty, and the code proceeds to mark the document as "Analysed by AI (No promises found)" with fullyProcessed: true. This records a permanent false negative instead of a recoverable failure.

} catch (error) {
  // ...logged as warn...
  if (APICallError.isInstance(error) || NoObjectGeneratedError.isInstance(error)) {
    continue; // ← silently drops chunk failure
  }
  throw error;
}

Fix: After the chunk loop, check if candidates.length === 0 when chunks were non-empty — if so, throw an error rather than continuing with empty candidates. Fix this →


P2 — Security / Design Concerns

3. API keys stored as plaintext in database (src/globals/Settings.ts:183-190)

The apiKey field in providerCredentials is a standard type: "text" field stored in MongoDB without encryption. This means provider API keys are readable by anyone with database access, CMS admin access, or through Payload's REST/GraphQL APIs.

Consider using a password/secret field type, environment-variable-only configuration for production, or at minimum documenting this limitation prominently.


4. Speculative model IDs in catalog (src/lib/ai/providerCatalog.ts:64-68)

The catalog includes model IDs that don't exist at the time of this PR (as of early 2026):

  • gpt-5, gpt-5-mini, gpt-5-nano — OpenAI has not released these
  • grok-4 — not available in xAI's API
  • claude-opus-4-1 — not a current Anthropic release

Including non-existent model IDs will fail silently at configuration time (they pass isProviderModelId validation since they match the format) and only fail at runtime with a cryptic provider error. Consider only including models that are currently GA, or add a comment explaining which entries are forward-looking.


5. as never cast bypasses type safety (src/lib/ai/providerRegistry.ts:297)

return createProviderRegistry(providers as never);

This completely disables TypeScript's type checking for the registry creation and hides any type incompatibilities between the provider objects and what createProviderRegistry expects. If the AI SDK's types change across a version bump, this will compile but break at runtime. The comment explaining why this cast is needed would help future maintainers.


P3 — Minor / UX Issues

6. All 14 providers instantiated per extraction (src/lib/ai/providerRegistry.ts:184-295)

buildLanguageModelRegistry eagerly instantiates every provider SDK client on every call, regardless of which provider is actually selected. For a background task this is minor overhead, but it also means credential resolution runs for all providers. Consider lazy instantiation or only building the provider that's actually needed.


7. Silent candidate truncation (src/tasks/extractPromises.ts:582-585)

When more than 220 candidates are found, the list is silently truncated (first 220 kept, rest dropped). There's a warn log, but no indication to the user or in the extraction result that promises may be missing. For documents with many distinct promises, this silently produces an incomplete extraction.


8. baseURL field hidden for all non-Ollama providers (src/globals/Settings.ts:202-206)

admin: {
  condition: (_, siblingData) => siblingData?.provider === "ollama",
  ...
},

OpenAI-compatible providers and Azure OpenAI deployments commonly need custom base URLs. The baseURL is stored in the schema and processed in the registry, but there's no way to configure it through the UI for providers other than Ollama. This is a hidden limitation — worth at least a comment or expanding the condition.


9. Ollama URL normalization is fragile (src/lib/ai/providerRegistry.ts:165-173)

const sanitized = baseURL.replace(/\/+$/, "");
return sanitized.endsWith("/v1") ? sanitized : `${sanitized}/v1`;

This will incorrectly handle URLs like http://localhost:11434/api (produces http://localhost:11434/api/v1) or http://localhost:11434/v1/chat (already contains /v1 but at a non-terminal position). Using URL parsing would be more robust.


10. stepCountIs(MAX_TOOL_STEPS) stops normalization silently (src/tasks/extractPromises.ts:352)

If the model hits 40 steps without calling finalizeExtraction, normalization stops and whatever promises have been recorded are used — which could be a partial list. There's no detection that finalizeExtraction was never called vs. the model completing normally. Worth checking hasToolCall in the result to detect this.


Nits

  • src/lib/ai/providerCatalog.ts:35MODEL_ID_PATTERN uses /^[a-z0-9-]+:\S+$/i but isProviderModelId also validates the provider against the catalog. The regex alone could match unknown-provider:some-model, which is harmless since the catalog check catches it, but could be tightened.
  • src/globals/Settings.ts:52 — The Settings export is a good opportunity to add JSDoc noting the legacy fields are intentionally hidden (not just deprecated in label).
  • No tests are included or referenced in the PR. The __extractPromisesTestUtils export suggests test infrastructure exists — shipping tests for the chunking, merging, and normalization logic would be valuable given the complexity of the two-pass approach.

Summary: The architecture is solid and the backward compatibility path is well thought out. The two P1 issues (data loss during force re-extract and false-negative on all-chunk-failure) should be addressed before merge. The security concern around plaintext API key storage and the speculative model IDs in the catalog are also worth resolving or at least explicitly acknowledging.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b1afc646b

ℹ️ 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".

Comment thread src/tasks/extractPromises.ts
…model enforcement

- Add API key validation for AI providers with caching and timeout
- Generate provider model IDs from installed AI SDK packages
- Restrict extraction tasks to models with structured output + tool calling support
- Reorganize Settings global into modular tab components (AI, Airtable, Meedan)
- Add script to regenerate provider model IDs from type definitions
- Improve provider credential configuration with real-time validation
@kelvinkipruto
Copy link
Copy Markdown
Contributor Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b7bf5b8cf3

ℹ️ 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".

Comment thread src/tasks/extractPromises.ts Outdated
Comment thread src/tasks/extractPromises.ts
…ponent

Move Settings.ts barrel export to direct import and create dedicated RowLabel component for AI provider rows. This improves component organization and enables custom row labeling in the admin UI.

- Delete barrel export file to simplify imports
- Add AIProviderRowLabel component for custom row display in array fields
- Update admin import map to include new component
- Configure AI credentials array to use custom RowLabel component
- Refactor arrow function formatting for consistency
@kelvinkipruto
Copy link
Copy Markdown
Contributor Author

@claude Review

@claude
Copy link
Copy Markdown

claude Bot commented Mar 3, 2026

Claude finished @kelvinkipruto's task in 2m 30s —— View job


Code Review

  • Gathering context and reading changed files
  • Reviewing provider catalog and registry
  • Reviewing extraction logic changes
  • Reviewing settings and credential validation
  • Compiling review feedback

This is a well-architected feature. The provider catalog/registry separation is clean, backward compatibility with legacy Google-only config is gracefully preserved, and the two-pass extraction with chunking is a meaningful improvement. Some issues from previous reviews are already addressed in the current code, which is great. Below are the remaining concerns.

Status of Previously Flagged Issues

Already addressed in the current code:

  • stepCountIs(MAX_TOOL_STEPS) silent truncation — normalizeCandidatesWithTools now throws if finalizeExtraction was never called (extractPromises.ts:356-364)
  • as never cast in createProviderRegistry — removed in current code (providerRegistry.ts:309)

P1 — Data Loss

1. Existing extractions deleted before new extraction completes (src/tasks/extractPromises.ts:534-550)

On forceReextract, all prior ai-extractions records are deleted before the new AI run begins. If any step fails after this point (empty text, API error, normalization failure), the document ends up with no extraction history and a failed status — permanently erasing previously valid data.

// Deletions happen here...
for (const extraction of existingExtractions) {
  await payload.delete({ collection: "ai-extractions", id: extraction.id });
}

// But extraction can still fail below — no rollback possible
const extractedCandidates = await extractPromisesFromChunks({...});

Fix: Move deletions to after payload.create succeeds (~line 633), or collect IDs and delete only on success. Fix this →


2. All-chunk failures silently become "No promises found" (src/tasks/extractPromises.ts:286-294)

Per-chunk APICallError and NoObjectGeneratedError are caught and swallowed. If every chunk fails (e.g. provider outage, rate limiting), candidates remains empty and the code proceeds through normalization — ultimately marking the document as fullyProcessed: true with "No promises found". This is a false negative that looks like a successful extraction.

if (APICallError.isInstance(error) || NoObjectGeneratedError.isInstance(error)) {
  continue; // ← all chunks could fail here with no signal
}

Fix: After the chunk loop, if chunks.length > 0 && candidates.length === 0, throw an error to let the outer catch handle it as a recoverable failure. Fix this →


P2 — Security / UX

3. API keys stored as plaintext (src/globals/Settings/tabs/AITab.ts:501-512)

The apiKey field uses type: "text", which means provider API keys are stored in plaintext in MongoDB. Anyone with database access, CMS admin access, or access via Payload's REST/GraphQL APIs can read them. At minimum, this limitation should be clearly documented in the field description. Ideally, use a type: "password" field type or enforce env-variable-only key configuration for production deployments.


4. Validation blocks saving when network is unavailable (src/globals/Settings/tabs/AITab.ts:200-210, src/lib/ai/providerCredentialValidation.ts:90-92)

When validateProviderApiKey returns "skipped" (timeout, connection refused, unknown endpoint behaviour), validateProviderCredentialApiKey returns an error string — preventing the settings form from saving. This means users on restricted networks cannot save settings even when their API key is valid, since the validation endpoint is simply unreachable.

if (status === "skipped") {
  return `Could not verify API key for "${getProviderDisplayName(providerId)}". Check network/base URL and try again.`;
}

Fix: Treat "skipped" as a non-blocking warning rather than a validation failure. Fix this →


5. Google API key exposed in URL query string (src/lib/ai/providerCredentialValidation.ts:122-124)

const endpoint = `https://generativelanguage.googleapis.com/v1beta/models?key=${encodeURIComponent(apiKey)}`;

Appending API keys as URL query parameters means they can appear in server access logs, browser history, and proxy logs. Prefer passing the key as a header (e.g. x-goog-api-key) if the Google API supports it, or at least document this in the code.


P3 — Minor / Design

6. All 14 providers eagerly instantiated per extraction (src/lib/ai/providerRegistry.ts:187-309)

buildLanguageModelRegistry creates SDK clients for all 14 providers on every call — including providers with no credentials. For a background task this is low overhead, but resolveConfiguredLanguageModel calls this to resolve a single model, making the full instantiation wasteful. Consider resolving only the required provider. Fix this →


7. trimToUndefined duplicated in three files

Identical implementations exist in providerCatalog.ts:69, providerRegistry.ts:54, and AITab.ts:80. Worth extracting to a shared utility.


8. baseURL field hidden for all non-Ollama providers (src/globals/Settings/tabs/AITab.ts:521-527)

condition: (_, siblingData) => siblingData?.provider === "ollama",

The baseURL is stored in the schema and processed in the registry for all providers, but there's no way to set it via the UI for non-Ollama providers. This blocks use cases like Azure OpenAI deployments or OpenAI-compatible self-hosted endpoints. Worth expanding the condition or adding a comment about the limitation.


9. Fragile Ollama URL normalization (src/lib/ai/providerRegistry.ts:177-185)

const sanitized = baseURL.replace(/\/+$/, "");
return sanitized.endsWith("/v1") ? sanitized : `${sanitized}/v1`;

http://localhost:11434/api becomes http://localhost:11434/api/v1, and http://localhost:11434/v1/chat already contains /v1 mid-path. Using URL parsing would be more robust here.


10. Silent candidate truncation not surfaced to users (src/tasks/extractPromises.ts:594-608)

When >220 candidates are found, the list is silently truncated. The warn log is good, but there's no indication in the stored extraction result or the document status that promises may be missing. For large documents, this can produce quietly incomplete extractions.


Nits

  • src/lib/ai/providerModelIds.generated.ts contains forward-looking IDs from the installed SDK types (e.g. gemini-3-pro-preview, grok-4). These come from the SDK's own type definitions so the generation approach is sound — but worth noting in a comment that the list reflects what the SDK declares, not necessarily what's deployed.
  • RowLabel.tsx renders data.data.provider directly with no fallback. If provider is undefined (new/empty row), the label shows blank rather than a placeholder like "New provider".
  • No tests are included. The __extractPromisesTestUtils export suggests test infrastructure exists — shipping tests for chunking, merging, and normalization logic would be valuable given the complexity.

Summary: The two P1 issues (data loss during forceReextract and all-chunk-failure false negatives) are the most important to address before merge. The "skipped" validation blocking save (P2 #4) is also a real UX regression worth fixing. The rest are improvements that can follow up.

- Add reusable MaskedApiKeyField component for secure API key display in settings tabs
- Extract trimToUndefined utility to shared module for consistent string handling
- Improve Google API key validation to use header-based authentication
- Enhance AI promise extraction to handle chunk failures and defer cleanup until successful re-extraction
- Add warning about generated model IDs possibly not being deployed in provider accounts
- Fix Ollama base URL normalization to properly handle path segments
- Simplify provider registry by directly creating language models without intermediate registry
@kelvinkipruto
Copy link
Copy Markdown
Contributor Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b8790d23d7

ℹ️ 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".

Comment thread src/tasks/extractPromises.ts Outdated
Comment thread src/globals/Settings/tabs/AITab.ts Outdated
@kelvinkipruto
Copy link
Copy Markdown
Contributor Author

@claude Review

@claude
Copy link
Copy Markdown

claude Bot commented Mar 3, 2026

Claude finished @kelvinkipruto's task in 2m 24s —— View job


Code Review

  • Gathering context and reading changed files
  • Reviewing extraction logic (extractPromises.ts)
  • Reviewing provider registry and catalog
  • Reviewing settings and credential validation
  • Compiling review feedback

This is a well-architected feature that has improved substantially across review iterations. Multiple P1/P2 issues from prior reviews have been resolved. Below is the current state.

Status of Previously Flagged Issues

Resolved in current code:

  • ✅ Data deleted before new extraction succeeds — finalizeForceReextract() is now deferred and only called after payload.create succeeds (extractPromises.ts:640-681)
  • ✅ All-chunk failures produce false negatives — explicit check added at extractPromises.ts:300-308 that throws when recoverableChunkFailures === chunks.length
  • stepCountIs(MAX_TOOL_STEPS) silent truncation — throws if finalizeExtraction never called (extractPromises.ts:368-376)
  • as never cast in provider registry — removed; direct per-provider instantiation used instead (providerRegistry.ts:204-257)
  • ✅ All 14 providers eagerly instantiated — resolveConfiguredLanguageModel now instantiates only the needed provider
  • trimToUndefined duplicated in 3 files — consolidated into stringUtils.ts, properly imported everywhere
  • ✅ Fragile Ollama URL normalization — now uses URL parsing (providerRegistry.ts:169-201)
  • ✅ Google API key exposed in URL query string — now uses x-goog-api-key header (providerCredentialValidation.ts:119)

Remaining Issues

P1 — Stale extractions after partial cleanup failure (src/tasks/extractPromises.ts:640-681)

When finalizeForceReextract encounters deletion errors, it logs a warning and continues. The new extraction has been successfully written, but any extractions that failed to delete remain in the database alongside it. If any downstream code (e.g. uploadToMeedan) queries ai-extractions by document ID, it will see both old and new records and could re-upload stale promises.

} catch (deletionError) {
  failedDeletes += 1;
  logger.warn({ ... }); // ← old extraction remains; no compensating action
}

The logged failedDeletes count is available but never acted upon. At minimum, the extraction metadata or document status should reflect incomplete cleanup so operators can identify affected documents. Fix this →


P2 — "skipped" validation blocks settings saves (src/globals/Settings/tabs/AITab.ts:196-199)

When validateProviderApiKey returns "skipped" (timeout, unreachable endpoint, connection refused), the field validator returns a hard error string — preventing the settings form from saving entirely. This blocks admins from saving valid credentials in environments with restricted outbound access or during transient provider outages.

if (status === "skipped") {
  return `Could not verify API key for "${getProviderDisplayName(providerId)}". Check network/base URL and try again.`;
}

"skipped" means "couldn't check" — not "key is wrong". It should be treated as a non-blocking warning (return true) and surfaced differently if at all. Fix this →


P2 — API keys stored as type: "text" despite PasswordField UI (src/globals/Settings/tabs/AITab.ts:493-508)

MaskedApiKeyField correctly renders a PasswordField component for masked input — but the underlying Payload field is still type: "text". Payload determines storage by field type, not component. The value is stored and returned as plaintext in Payload's REST/GraphQL APIs and is readable from MongoDB without any encryption. Anyone with CMS admin access or database access can read all provider API keys in cleartext. This limitation should be documented prominently in the field description.


P2 — baseURL UI hidden for all non-Ollama providers (src/globals/Settings/tabs/AITab.ts:517-524)

condition: (_, siblingData) => siblingData?.provider === "ollama",

The baseURL is processed in the registry for all providers (providerRegistry.ts:215) but can only be set via UI for Ollama. Azure OpenAI deployments, self-hosted OpenAI-compatible endpoints, and custom Anthropic proxy URLs all require custom base URLs and have no UI path to configure them. The field description currently says "Optional Ollama base URL" which doesn't hint at this limitation for other providers. Fix this →


Minor / Nits

Dynamic model ID loading reads the filesystem at module init (src/lib/ai/providerCatalog.ts:277-285)

AI_PROVIDER_CATALOG is a top-level const that calls loadProviderModelIdsreadTypeUnionLiteralsfs.readFileSync for each provider with a type source (13 out of 14) during module initialization. The in-process cache (providerModelIdsCache) prevents repeated reads within a single process, but in serverless environments with frequent cold starts, this runs on every cold start. The generated fallback file is a sensible mitigation; consider preferring it by default and using the filesystem read only as an update mechanism.

validateModelPreset error message references openai:gpt-5 (src/globals/Settings/tabs/AITab.ts:379)

return 'Use format "provider:model", for example "openai:gpt-5".';

gpt-5 is not yet a real model. Using an existing model like openai:gpt-4o as the example would avoid confusion.

Silent candidate truncation not surfaced in stored data (src/tasks/extractPromises.ts:599-614)

candidateListWasTruncated is logged and reflected in the Airtable status string, but the ai-extractions record itself doesn't store a flag. If admins query extractions directly or build reporting on them, they can't distinguish complete from truncated extractions without checking Airtable.

No tests included__extractPromisesTestUtils exports are already in place, suggesting test infrastructure was intended. The chunking, merging, all-chunks-fail, and normalization-timeout flows all have meaningful edge cases that would benefit from unit tests.


Summary: The architecture is solid and the most critical issues from previous reviews have been addressed. The remaining P1 (stale extractions on partial cleanup failure) and P2 ("skipped" blocking saves, plaintext API key storage, hidden baseURL for non-Ollama) are the main items left to address before merge.

@kelvinkipruto
Copy link
Copy Markdown
Contributor Author

@claude Review

@claude
Copy link
Copy Markdown

claude Bot commented Mar 3, 2026

Claude finished @kelvinkipruto's task in 2m 26s —— View job


Code Review

  • Gathering context
  • Reviewing extraction logic (extractPromises.ts)
  • Reviewing provider registry and catalog
  • Reviewing settings and credential validation
  • Compiling review feedback

This PR has significantly improved across review iterations. Most previously flagged P1 and P2 issues are now resolved. Below is the current status.

Previously Flagged Issues — Now Resolved

  • Data loss on force re-extractfinalizeForceReextract() is now called only after payload.create succeeds (extractPromises.ts:726). If cleanup fails, the function throws and the catch block rolls back the newly created extraction (extractPromises.ts:767-799).
  • All-chunk failures produce false negatives — explicit check added at extractPromises.ts:300-308 that throws when recoverableChunkFailures === chunks.length.
  • stepCountIs(MAX_TOOL_STEPS) silent truncation — throws if finalizeExtraction was never called (extractPromises.ts:368-376).
  • Stale extractions on partial cleanup failurefinalizeForceReextract now throws if failedDeletes > 0, triggering rollback of the new extraction.
  • "skipped" validation blocks savesvalidateProviderCredentialApiKey now only returns an error for "invalid", treating "skipped" as non-blocking (AITab.ts:192-196).
  • Google API key in URL query string — now uses x-goog-api-key header (providerCredentialValidation.ts:119).
  • Fragile Ollama URL normalization — replaced with URL-based parsing (providerRegistry.ts:176-202).
  • as never cast in provider registry — removed; per-provider switch used instead.
  • All 14 providers eagerly instantiatedresolveProviderLanguageModel now only creates the needed provider client.
  • trimToUndefined duplication — consolidated into stringUtils.ts.
  • RowLabel shows blank for undefined provider — falls back to "New provider" (RowLabel.tsx:10).

Remaining Issues

P2 — API keys stored as type: "text" despite MaskedApiKeyField UI (src/globals/Settings/tabs/AITab.ts:486-502)

MaskedApiKeyField correctly renders the field as a password input for UI masking, but the Payload field declaration remains type: "text". Payload uses the field type to determine serialization and access — a type: "text" field is returned in plaintext via Payload's REST and GraphQL APIs, and is stored without any encryption in MongoDB. Anyone with CMS admin access or database access can read provider API keys as cleartext. At minimum, this limitation should be documented prominently in the field description.


P2 — baseURL field only visible for Ollama (src/globals/Settings/tabs/AITab.ts:510-517)

condition: (_, siblingData) => siblingData?.provider === "ollama",

The baseURL is processed in the provider registry for all providers (providerRegistry.ts:217-219), but there is no UI path to set it for non-Ollama providers. Azure OpenAI deployments, self-hosted OpenAI-compatible endpoints, and Anthropic proxy configurations all need custom base URLs and are silently unsupported via the UI. The field description "Optional Ollama base URL" further obscures this. Worth either expanding the condition to include other providers (e.g. openai, anthropic) or at least updating the description to note the limitation. Fix this →


Minor / Nits

Example model ID in error message is a non-existent model (src/globals/Settings/tabs/AITab.ts:417)

return 'Use format "provider:model", for example "openai:gpt-5".';

gpt-5 does not exist at the time of this PR (early 2026). Using a real model such as openai:gpt-4o in the example avoids confusing admins who try to copy it verbatim. Fix this →


Silent candidate truncation not stored in extraction record (src/tasks/extractPromises.ts:606-622)

candidateListWasTruncated is logged and reflected in the Airtable status string, but the ai-extractions record itself has no flag for it. Downstream code or reporting tools cannot distinguish a complete extraction from one where promises were silently dropped. Adding a boolean field to the stored record would allow this to be surfaced.


Dynamic model ID loading reads filesystem at module init (src/lib/ai/providerCatalog.ts:277-285)

AI_PROVIDER_CATALOG is a top-level const that triggers fs.readFileSync for 13 provider packages at module initialization. The in-process cache (providerModelIdsCache) avoids repeated reads within a session, but in serverless environments with frequent cold starts, this runs on every invocation. The generated fallback file (providerModelIds.generated.ts) is a good mitigation — consider preferring it and using the filesystem read only as a build-time update mechanism.


No tests included__extractPromisesTestUtils exports are already in place. The chunking, merging, all-chunks-fail, and normalization-timeout edge cases all have non-obvious behavior that would benefit from unit test coverage, especially given the rollback logic introduced in this PR.


Summary: The architecture is solid and the codebase has improved substantially through the review cycle. The remaining items are a P2 security note about plaintext API key storage, a UI gap for non-Ollama baseURL configuration, and a handful of nits. These don't block functionality but are worth addressing before merge — particularly the API key storage limitation being documented if not fully mitigated.

@kelvinkipruto kelvinkipruto merged commit dbcd8fc into main Mar 6, 2026
6 checks passed
@kelvinkipruto kelvinkipruto deleted the ft/mutliprovider-ai-llms branch March 6, 2026 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Promise Tracker | AI Promise extraction prompt logic fine-tuning | Split Multi-Promise Text into Unique Atomic Records

1 participant