Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@claude review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04c50baa99
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Prior feedback addressed in b975638; threshold suffix is now conditional and covered by a new test.
Extended reasoning...
Overview
This PR adds a RunnerContext wrapper class around langfuse.experiment.run (with CI-injected defaults for data, datasetVersion, and metadata) and a RegressionError class for signaling CI gate failures. It also adds the corresponding test file and re-exports the new module from the client package index. Scope is small (3 files, ~410 lines mostly new).
Security risks
None. The code is a pure client-side wrapper with no I/O, no auth, no crypto, no input parsing of untrusted data. metadata is shallow-merged with call-time keys winning; data is passed through unchanged to the existing experiment runner.
Level of scrutiny
Low-to-medium. This is new, additive, public API surface (@public JSDoc) intended to pair with the langfuse/experiment-action GitHub Action and mirror the Python SDK RunnerContext. The logic is straightforward (default resolution + metadata merge + error formatting), and behavior is well-covered by the new unit tests.
Other factors
The single P1/P2 finding raised by Greptile, the Codex bot, and my prior review (rendering (threshold undefined) when threshold is omitted from the structured form) was fixed in b975638 with a conditional template suffix and an accompanying omits the threshold suffix when no threshold is provided test. Type-level negative cases (@ts-expect-error) are also exercised. No outstanding review threads remain unresolved.
1 participant
Summary
Add a JS SDK
RunnerContextwrapper for experiment-action style flows, plus aRegressionErrorexport for CI regression signaling.Disclaimer: Experimental PR review
Greptile Summary
This PR adds a
RunnerContextclass that wrapslangfuse.experiment.runwith CI-injected defaults (data, datasetVersion, metadata), and aRegressionErrorclass for signaling CI gate failures — the JS equivalent of the Python SDK'sRunnerContext.thresholdis omitted:RegressionErrorMetricOptionsdeclaresthresholdas optional, but the structured message template always interpolates it, producing\"(threshold undefined)\"when onlymetricandvalueare supplied.Confidence Score: 3/5
One P1 bug produces a misleading error message for a valid, type-safe usage of RegressionError; fix is a one-line template-literal change before merging.
A single P1 finding with a straightforward fix caps confidence at 4; the bug is in a public-facing API surface (RegressionError.message) and has no existing test coverage for the affected path, pulling the score to 3.
packages/client/src/experiment/RunnerContext.ts — threshold interpolation in RegressionError constructor
Important Files Changed
Sequence Diagram
sequenceDiagram participant CI as CI / GitHub Action participant RC as RunnerContext participant EM as LangfuseClient.experiment participant LF as Langfuse API CI->>RC: new RunnerContext({ client, data, datasetVersion, metadata }) CI->>RC: runExperiment({ name, task, [overrides] }) RC->>RC: resolve data (call ?? ctx) RC->>RC: merge metadata (ctx + call, call wins) RC->>EM: experiment.run({ ...merged params }) EM->>LF: create dataset run items, scores LF-->>EM: ExperimentResult EM-->>RC: ExperimentResult RC-->>CI: ExperimentResult alt regression detected CI->>CI: throw new RegressionError({ result, metric, value, threshold? }) endPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "Merge branch 'main' into tobias/lfe-9365..." | Re-trigger Greptile