Repository navigation
feat(prompts): sync Langfuse prompts into the catalog - #17095
AtefBellaaj wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e84111146
ℹ️ 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".
1e84111 to
954182d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 954182db00
ℹ️ 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".
954182d to
5a35cfc
Compare
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
5a35cfc to
39abe55
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39abe556cc
ℹ️ 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".
39abe55 to
dc469c7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc469c75c5
ℹ️ 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".
| async function syncPrompts(req: ServerRequest, res: Response): Promise<Response> { | ||
| const gate = await gatePromptSyncTenantSwitch(res); | ||
| if (gate.response) { |
There was a problem hiding this comment.
When a single-tenant deployment uses LANGFUSE_PUBLIC_KEY and LANGFUSE_SECRET_KEY, isLangfuseConnectionAvailable() makes api/server/routes/config.js publish langfuseConnectionAccess: false, so LangfuseConnection never mounts. This new env-capable sync handler still requires promptSync.enabled from stored base config, while the only UI for setting that flag and starting the sync is in that hidden panel; operators therefore cannot use the supported UI and must manually call the admin API or preseed the database, despite env-mode synchronization being implemented and tested. Advertise a separate prompt-sync capability and surface these controls whenever sync is available.
AGENTS.md reference: AGENTS.md:L50-L53
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accurate description, but the gating is inherited rather than introduced here. The prompt-sync enable switch, the preview dialog and the sync button all live in the same promptSync.available block of LangfuseConnection.tsx, and that panel mounts only when langfuseConnectionAccess is true, which isLangfuseConnectionAvailable() sets to false in env-credential mode by design: in that mode Langfuse is operator-configured, not admin-configured. That rule and the enable toggle come from the base branch; this PR adds one button to the existing block. The sync endpoints are gated by LANGFUSE_PROMPT_SYNC_AVAILABLE and the stored promptSync.enabled flag, not by connection availability, so env-mode deployments can enable and run the sync through the admin API today, and langfuse.handler.spec.ts covers env mode end to end. An env-mode UI surface for prompt sync is a panel-level change tracked as a follow-up on the stack, not folded into this PR.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc469c75c5
ℹ️ 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".
c24ef6c to
c0c846c
Compare
c0c846c to
4d27e36
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8b14e1e41
ℹ️ 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".
c8b14e1 to
d31bc95
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d31bc95da4
ℹ️ 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".
| sourceProjectId: source.projectId, | ||
| sourcePromptName: item.name, | ||
| }; | ||
| const outcome = await syncOne(item, identity, author, deps); |
There was a problem hiding this comment.
When a page contains many new prompts, this await makes every item run serially: syncOne performs a lookup and insert, while the injected grantPermission also reads the role and writes the ACL before the next item begins. Consequently, the supported 60-second lease can expire before it is renewed after the page—for example, 100 new entries with 250 ms database latency require well over a minute—allowing another node to enter while the first continues writing. Fresh evidence beyond the earlier lease-configuration thread is that the extracted runner still performs this additional ACL work per item; use bounded page concurrency or bulk methods and renew the lease independently before it expires.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping the serial loop in this PR. Checked against d31bc95:
promptCatalogSync.tsis byte-identical to the feature commit 4d27e36, and that commit already granted the public viewer role per item from the handler (admin/langfuse.tsL905 there). The runner extraction moved the wiring, not the per-item work, so there is no new evidence beyond the earlier lease thread.- The default lease is 300 s (
DEFAULT_PROMPT_SYNC_LOCK_LEASE_MSinpolicy.ts), not 60 s. 60 s is the clamp floor;langfuse.promptSync.lockLeaseMsinlibrechat.yamlaccepts up to 3 600 000. A page of 100 new prompts costs at most four round trips each (find, insert, role read, ACL upsert): 400 round trips at the quoted 250 ms is 100 s, inside the default lease. Prompts that already exist cost one round trip. - If a lease does lapse, the second node runs the same find-then-insert: its inserts of groups the first node already created fail with duplicate key 11000 and are reported as
unchanged, and the first node returnslock_lostat the next page boundary (promptCatalogSync.tsL187-191). The lock deduplicates work; the unique index keeps the catalog consistent. - AGENTS.md L187-190 addresses startup and request paths. This is an admin-triggered batch over a paged external API, where bounded fan-out trades predictable Mongo load for bursts and complicates the per-item rollback (
deletePromptGroupafter a failed grant).
Renewing the lease on an item count inside a page is a reasonable follow-up. It changes lock semantics in the feature commit and stays out of this refactor commit.
d31bc95 to
785c9d0
Compare
|
@codex review a1fa67301 |
785c9d0 to
7eb79d8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7eb79d8c2f
ℹ️ 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".
| function isDuplicateKeyError(error: unknown): boolean { | ||
| return (error as { code?: number } | undefined)?.code === 11000; |
There was a problem hiding this comment.
This runner directly interprets MongoDB error code 11000 (and the adjacent predicate recognizes Mongoose's ValidationError), so the exported packages/api integration now owns storage-specific failure semantics. A different repository implementation would require changing this API module rather than supplying a plain contract; classify these failures in packages/data-schemas and return or inject stable, storage-neutral outcomes instead.
AGENTS.md reference: AGENTS.md:L74-L76
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping these predicates here.
- They are not new in this PR. At the feature commit (7480cc3) the same
isDuplicateKeyErrorandisValidationErrorlived inpackages/api/src/admin/langfuse.tsand were passed into the sync. The refactor commit relocates them into the composition module; it does not introduce storage semantics intopackages/api. promptSyncRunner.tsis the composition point that binds the Mongo-backed prompt-group methods and the ACL grant to the storage-neutral core.syncLangfusePromptCataloginpromptCatalogSync.tstakes both predicates by injection and knows nothing about Mongo. A different repository implementation supplies a different composition function with its own predicates, which is the plain contract the finding asks for.- There is no data-schemas helper for this today. Nine other production files under
packages/api/srctestcode === 11000inline (agents/handlers.ts, agents/triggers/bindings.ts, auth/email.ts, auth/passkey/handlers.ts, code/http.ts, middleware/error.ts, skills/handlers.ts, skills/import.ts). Introducing a shared classifier inpackages/data-schemasis a new convention for all of them, not a fix local to this PR, and AGENTS.md asks to stop widening rather than rewrite what exists.
If a shared duplicate-key classifier is wanted, it is a separate change that migrates those call sites together.
7eb79d8 to
4e94232
Compare
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…e admin handler Add createLangfusePromptSyncRunner, which binds the lock, the prompt-group methods, the ACL grant and the Mongo error predicates once and returns a runner that takes the per-request source, connection-bound listPrompts, lock lease and page cap. The admin handler receives that runner as one dependency instead of forwarding its parts, and its config method signatures return a LangfuseAdminConfig interface in place of Mongoose document types. In api, api/cache/distributedLock.js creates the one DistributedLock for the process and api/server/services/LangfusePromptSync.js composes the runner once with it; the admin route imports the composed runner.
4e94232 to
c73fb10
Compare
This branch has not been deployed
1 participant
Pull Request
Summary
Today a Langfuse text prompt is only previewable through the connection's dry run; nothing it returns ever lands in the prompt library, so an admin who wants to actually use one of those prompts has to copy it by hand into a native PromptGroup. This change lets an admin click Sync prompts on Settings > Langfuse, or POST /api/admin/langfuse/prompts/sync directly, to pull every Langfuse text prompt into the catalog as a read-only PromptGroup pointer, each carrying a PUBLIC viewer ACL entry so every user can see it without an explicit grant.
After the change, a sync runs inside the request: it creates one PromptGroup per Langfuse text prompt and attaches the PUBLIC viewer entry; prompts of other types are skipped. The sync is append-only — it creates pointers for new Langfuse text prompts and never deletes, re-grants, or reconciles an existing group, so a prompt later deleted in Langfuse keeps its local pointer, and a second sync against the same tenant changes nothing. Per item, a duplicate-key race against an existing pointer counts as unchanged, a Mongoose validation error counts as skipped and is warned, and any other storage or grant failure aborts the run (a failed grant rolls back the group it just created) and the endpoint returns 500
{ code: 'internal' }; a Langfuse transport failure instead surfaces its ownupstreamcode, and a caller without the capability sees "You do not have permission to manage prompts." A sync already running for the tenant returns 409already_runninginstead of starting a second pass. If the distributed lock is lost mid-run (lease expired, another node took over), the handler returns 503lock_lostrather than continuing to write with a stale claim; a failed lock release is only logged, since the lease still expires on its own. The lock lease and the page cap are both configurable onlibrechat.yaml(langfuse.promptSync.lockLeaseMs, default 300000 ms, range 60000–3600000;langfuse.promptSync.maxPages, default 50 pages of up to 100 prompts each, range 1–1000), not an environment variable or a hard-coded constant. A new non-unique partial index on{ sourceDestination, sourceProjectId, sourcePromptName }backs the lookup for a sync running without tenant context, so it still gets an index scan rather than a collection scan. Deleting the admin account that ran the sync does not delete the groups it created — they are owned by a synthetic source author, not the triggering user. The startup migration check and the config/migrate-prompt-permissions.js CLI both exclude already-synced groups from the backfill they look for, since those groups are intentionally external and read-only rather than missing a migration.How it works
sequenceDiagram participant Admin participant Route as POST /prompts/sync participant Lock participant Langfuse participant Catalog as PromptGroup store Admin->>Route: trigger sync Route->>Route: check capabilities + resolve tenant Route->>Lock: acquire(tenant key, leaseMs) loop each Langfuse page (type=text, up to maxPages) Route->>Langfuse: list prompts Langfuse->>Route: prompt page Route->>Catalog: find or insert PromptGroup Route->>Catalog: grant PUBLIC viewer ACL opt another page remains and the page cap is not yet reached Route->>Lock: extend(token, leaseMs) end end Route->>Lock: release(token) Route->>Admin: sync summaryType of change
Testing
Tested environments/configuration:
Automated tests:
packages/api: 464 tests passing across 15 suites (includes the route-dependency test file: 20 tests; the admin handler spec: 119; the Langfuse prompt-sync/catalog/policy suites: 29 + 26; the migration spec: 4),npx tsc --noEmitcleanpackages/data-schemas:src/methods/prompt.spec.ts, 36 tests passing,npx tsc --noEmitcleanapi:server/routes/admin/langfuse.test.js, 27 tests passingconfig: migration CLI specs, 15 tests passingclient:src/components/Nav/SettingsTabs/Integrations+src/data-provider/Langfuse, 58 tests passing (4 suites),npx tsc --noEmitcleannpx prettier --check,eslint, andnpm run sort-importsall clean on touched pathsManual (not yet run):
Screenshots / recordings
No screenshots attached. The only visible change is a Sync prompts button in Settings > Langfuse and its result toast; no layout, theming, or motion change.
Risk / compatibility
Checklist
🤖 Generated with Claude Code