fix: track scalar provider dependencies - #979
Conversation
|
@codex review Please review exact head |
There was a problem hiding this comment.
Pull request overview
This PR fixes a dependency-tracking gap in the action’s Promptfoo config handling: providers can be a scalar string (e.g., providers: file://provider.js), and the action previously treated it as array-only, leading to missed provider dependencies and potentially skipped evaluations.
Changes:
- Update the config model to accept
providersas either a scalar string or an array of provider entries. - Normalize scalar
providersinto a single-element array before running the existing dependency extraction logic. - Add test coverage for scalar provider cases and regenerate the bundled
dist/index.js.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/utils/config.ts | Accepts scalar providers and normalizes to an array for dependency extraction. |
| tests/utils/config.test.ts | Adds tests for scalar providers (file, glob, and non-file). |
| dist/index.js | Regenerated bundle reflecting the source change; also alters the __commonJS helper behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Nice work! 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". |
…alar-provider-dependencies # Conflicts: # dist/index.js.map
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
|
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 Please re-review exact head |
![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: 4fa94d5db8
ℹ️ 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".
|
@codex review Please re-review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fca13f896c
ℹ️ 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".
| if (config.scenarios) { | ||
| for (const scenario of config.scenarios) { | ||
| if (typeof scenario === 'string') { | ||
| extractTests(scenario); |
There was a problem hiding this comment.
When a config uses external scenario files (scenarios: - file://scenarios/*.yaml; Promptfoo docs: https://www.promptfoo.dev/docs/configuration/scenarios/), this path loads the scenario YAML through extractTests, which only understands test-case shapes and never visits the loaded scenario's config or tests. As a result, files referenced only inside those scenario files (vars, assertions, providers, etc.) are absent from dependencies, so a PR touching one of them can hit the “No LLM prompt...” skip instead of re-evaluating; handle loaded scenario objects with the same traversal as inline scenarios.
Useful? React with 👍 / 👎.
| processFileUrl( | ||
| (renderedPromptPath.startsWith('file://') | ||
| ? renderedPromptPath | ||
| : `file://${renderedPromptPath}` | ||
| ).replace(PROMPT_FILE_SELECTOR_PATTERN, '$1'), | ||
| ); |
There was a problem hiding this comment.
When prompts contains a string YAML prompt such as file://prompts/chat.yaml, this call records the prompt file but discards the returned path, so structuredPromptFiles is populated only by the { file: ... } branch. Promptfoo's YAML prompt processor recursively resolves nested file:// references (https://raw.githubusercontent.com/promptfoo/promptfoo/main/src/prompts/processors/yaml.ts), so a PR changing only one of those nested files can still be skipped even though the eval uses it; add resolved YAML prompt paths from this branch to the structured prompt scan.
Useful? React with 👍 / 👎.
| if (character === ']') { | ||
| inCharacterClass = false; | ||
| } |
There was a problem hiding this comment.
If a workflow uses a supported POSIX character class such as prompts/[[:alpha:]]*.txt, this validator exits the character class at the inner ] and then treats the outer ] as malformed, so action prompt globs fail before glob.sync even though minimatch documents [[:alpha:]] as valid syntax (https://isaacs.github.io/minimatch/). This blocks valid prompt/dependency patterns; the delimiter scan needs to recognize nested POSIX class brackets while inside [...].
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8883a9219f
ℹ️ 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".
| const prompts = Array.isArray(config.prompts) | ||
| ? config.prompts | ||
| : typeof config.prompts === 'string' | ||
| ? [config.prompts] | ||
| : Object.keys(config.prompts); |
There was a problem hiding this comment.
When prompts is a map, Promptfoo treats the map values as the prompt definitions and the keys as labels, but this branch only iterates Object.keys(config.prompts). In a config like prompts: { chat: file://prompts/chat.yaml }, dependency extraction sees only chat and never records prompts/chat.yaml, so a PR that changes only that config-defined prompt can still hit the skip path when prompt filtering is enabled.
Useful? React with 👍 / 👎.
| for (const key of ['transformVars', 'transform', 'postprocess']) { | ||
| const transformValue = options[key]; | ||
| if ( | ||
| typeof transformValue === 'string' && | ||
| transformValue.startsWith('file://') | ||
| ) { |
There was a problem hiding this comment.
This only checks transform-related option keys, but Promptfoo also loads provider-specific files from the same options object, e.g. options.response_format: file://schemas/math-response.json or nested schemas. In configs that rely on per-test provider options, a schema/request file change is not added to dependencies, so the action can skip evaluation even though Promptfoo will load different runtime config.
Useful? React with 👍 / 👎.
| const closers: Record<string, string> = { | ||
| '{': '}', | ||
| '(': ')', | ||
| '[': ']', |
There was a problem hiding this comment.
Because ( is treated as a delimiter everywhere, an exact prompt pattern for a valid filename such as prompts/foo (draft.txt fails this validation before glob.sync can match it. Minimatch's extended glob syntax only uses parentheses as part of operators like +(a|b), so workflows that monitor prompt files with literal unmatched parentheses now fail with “malformed delimiters” instead of evaluating the changed prompt.
Useful? React with 👍 / 👎.
| if (config.extensions) { | ||
| const extensions = Array.isArray(config.extensions) | ||
| ? config.extensions | ||
| : [config.extensions]; | ||
| for (const extension of extensions) { | ||
| if (extension.startsWith('file://')) { |
There was a problem hiding this comment.
When a config sets hooks via commandLineOptions.extension, Promptfoo treats those as default CLI extension files and loads them during promptfoo eval, but this extractor only scans top-level extensions. With prompt filtering enabled, a PR that changes only such a hook file is not marked as a dependency change and can still be skipped; route commandLineOptions.extension through the same tracking path.
Useful? React with 👍 / 👎.
| /\.(?:cjs|cts|j2|js|json|jsonl|md|mjs|mts|py|ts|txt|ya?ml)(?::[^:]*)?$/.test( | ||
| prompt, | ||
| ) || | ||
| prompt.charAt(prompt.length - 3) === '.' || | ||
| prompt.charAt(prompt.length - 4) === '.' || |
There was a problem hiding this comment.
When a config references a same-directory executable prompt as prompts: generate.bash without file:// or ./, Promptfoo auto-detects common script extensions and executes that file, but this helper only recognizes the allowlisted extensions or 2/3-character suffixes. The .bash path is treated as inline prompt text here, so a PR that only changes generate.bash can still be skipped instead of re-evaluating.
Useful? React with 👍 / 👎.
| if (/\.csv$/i.test(testFile)) { | ||
| for (const match of testContent.matchAll( | ||
| /file:\/\/[^,"'\r\n\]}]+/g, |
There was a problem hiding this comment.
For external CSV tests, quoted fields may legally contain commas, but this regex stops a file:// dependency at the first comma. With an assertion value like "file://validators/check,case.cjs:validate", Promptfoo's CSV parser uses validators/check,case.cjs, while dependency extraction records only the truncated path, so changing the actual validator file can still be treated as unrelated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f42482cae7
ℹ️ 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".
| if (!filterPath.startsWith('file://')) { | ||
| processFileUrl(`file://${filterPath}`); |
There was a problem hiding this comment.
When a Nunjucks filter path starts with /, Promptfoo’s filter loader still joins it to the config base directory, so /filters/direct.js loads <configDir>/filters/direct.js. Prefixing that value with file:// here makes it look like an absolute filesystem path (file:///filters/direct.js), which the containment check rejects without adding any dependency; a PR that only changes that filter can therefore still be skipped. Treat these filter paths with the same base-directory semantics as Promptfoo instead of converting leading-slash values into absolute file URLs.
Useful? React with 👍 / 👎.
| ? config.extensions | ||
| : [config.extensions]; | ||
| for (const extension of extensions) { | ||
| if (extension.startsWith('file://')) { |
There was a problem hiding this comment.
When extensions uses the documented plain file-path form such as extensions: ['hooks.js:beforeEach'], Promptfoo still loads that hook file, but this branch only records entries that start with file://. With prompt filtering enabled, a PR that changes only the extension hook can still be treated as unrelated and skip the eval; handle plain extension paths and strip the function selector the same way as file:// hooks.
Useful? React with 👍 / 👎.
| for (const test of config.tests) { | ||
| extractVarFiles(test.vars); | ||
| extractAssertFiles(test.assert); | ||
| extractTests(config.defaultTest); |
There was a problem hiding this comment.
When defaultTest: file://defaults/base.yaml contains plain paths such as vars: vars.yaml or a provider script, Promptfoo loads the default file but then resolves those nested plain paths against the main config directory. Calling the generic external-test traversal here switches the base to defaults/, so the extractor records defaults/vars.yaml instead of the file Promptfoo actually uses; a PR changing only the config-root vars.yaml can still skip evaluation.
Useful? React with 👍 / 👎.
| const absoluteVarPath = path.isAbsolute(rawVarPath) | ||
| ? rawVarPath | ||
| : path.resolve(testBaseDir, rawVarPath); | ||
| processFileUrl(`file://${absoluteVarPath}`); |
There was a problem hiding this comment.
When a test uses a vars file (vars: vars.yaml) and that vars file contains file:// values, Promptfoo loads those nested files while expanding the vars file, but this branch only records the vars file itself. With prompt filtering enabled, a PR that changes only the nested fixture referenced from vars.yaml can still be classified as unrelated and skip the eval; read bounded YAML/JSON vars files the same way external tests/prompts are scanned, or conservatively watch the dependency root.
Useful? React with 👍 / 👎.
mldangelo
left a comment
mldangelo
left a comment
There was a problem hiding this comment.
Tracks scalar provider dependencies, provider file templates, env-built provider configs, nested credential deps, and HTTP multipart uploads. Notably, its outer catch returns ['./'] (conservatively watch the root) rather than [] — the safest error policy in this PR family. The provider traversal is the most complex of the seven, and it shows: processProviderValue threads 10 positional parameters through every recursive call, there are two parallel structures tracking the same membership, and the same idioms are duplicated many times. There's also the same file:// double-prefix bug in the credential branch as #974. Details inline.
Cross-cutting (the seven config.ts PRs): #974, #977, #979, #981, #982, #983, and #987 are seven independent branches off main that each rewrite the same extractFileDependencies in src/utils/config.ts (this file is 251 lines on main; these PRs add ~130–1000 diff lines each). They mutually conflict — only one can merge cleanly and the rest need large manual reconciliation — and they don't even agree on error semantics: on an unexpected parse error #974 throws (fails the action), most return [] (silently disables dependency tracking, which can miss a changed dependency), and #979 returns ['./'] (conservatively watches the root — the safest of the three). Strongly recommend consolidating this family into a single stacked/reviewable change with one agreed error policy (prefer #979's conservative-watch) and shared helpers, rather than merging seven overlapping rewrites.
Merge/stacking note: One of seven competing config.ts rewrites; overlaps heavily with #974 (mapped providers) and #987 (provider canonicalization) — three PRs independently reworking provider dependency extraction. Consolidate.
Additional findings (not on changed lines, noted here):
-
🟡 P3 · simplification — processProviderValue threads 10 positional parameters through every recursive call — in
src/utils/config.ts(neargreatGrandparentKey?: string,)Six positional booleans plus three manually-shifted ancestor-key strings (
parentKey/grandparentKey/greatGrandparentKey), mirrored again in a 9-fieldJSON.stringifycache key. Every ~10 recursive call site passes them positionally (processProviderValue(config.targets, false, configEnv, false, false, true)), so any new field means editing every call site and the cache key in lockstep — and it's the most likely place for a mis-ordered boolean to slip in. Collapse into a single context object with anancestorKeys: string[](.at(-1/-2/-3)); call sites then spread-and-override only what they change. -
🟡 P3 · simplification — activeProviderValues WeakSet is fully redundant with activeProviderValueEnvs WeakMap — in
src/utils/config.ts(nearconst activeProviderValues = new WeakSet<object>();)Both are added together, deleted together in the same finally, and the only read is
if (activeProviderValues.has(value)) { if (activeProviderValueEnvs.get(value) === envContextKey) … }. SinceenvContextKeyis always a JSON string,activeProviderValueEnvs.has(value)is an exact substitute. Delete the WeakSet and its.add/.deletelines; useactiveProviderValueEnvs.has(value)(−4 lines, one structure, no drift hazard).
| if ( | ||
| envValue === undefined || | ||
| (useFalsyDefault === 'true' && !envValue) | ||
| ) { |
There was a problem hiding this comment.
🟡 P3 · correctness — HTTP credential branch double-prefixes file:// (same bug as #974)
The fileAuthPath and multipart blocks guard with x.startsWith('file://') ? x : file://${x}, but the credential block unconditionally builds file://${credentialPath} ``. A credential field carrying a file:// value (or an env template that renders to one) becomes `file://file://…`, which `processFileUrl` resolves to a bogus `/file:/…` dependency, so a change to the real key file is missed. Extract one `trackRenderedFilePath` helper used by all three branches so the credential path gets the same guard — this also removes ~20 duplicated lines.
| absolutePath: string, | ||
| ): string | undefined => { | ||
| const containingRoot = getContainingDependencyRoot(absolutePath); | ||
| if (!containingRoot) { |
There was a problem hiding this comment.
🟡 P3 · simplification — Dependency-root sentinel expression duplicated 7×
`${dependencyRoot.replace(/[\\/]+$/, '')}${path.sep}` is written out seven times, though dependencyRoot is fixed at the top of the function, so the sentinel is a constant. Hoist const dependencyRootSentinel = …; next to the dependencyRoot declaration (or a tiny watchDependencyRoot() helper) and replace all seven occurrences — this family has already had to keep these copies aligned across revisions.
activeProviderValues (WeakSet) and activeProviderValueEnvs (WeakMap<object, string>) tracked identical membership — added, read, and deleted together. Since the map's values are always a JSON string, activeProviderValueEnvs.has() is an exact substitute for the WeakSet check. Remove the parallel structure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
31d2ccd to
33c901f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33c901ff49
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!filterPath.startsWith('file://')) { | ||
| processFileUrl(`file://${filterPath}`); |
There was a problem hiding this comment.
When a config uses an explicit filter path such as nunjucksFilters: { pretty: 'file://filters/pretty.js' }, Promptfoo still loads that filter module, but this condition only processes values that do not start with file://. The file:// case falls through without adding the filter file as a dependency, so with prompt filtering enabled a PR that changes only the filter can be skipped even though evaluation behavior changes.
Useful? React with 👍 / 👎.
| const rawPrompt = | ||
| typeof prompt === 'string' | ||
| ? prompt | ||
| : typeof prompt.raw === 'string' | ||
| ? prompt.raw | ||
| : typeof prompt.id === 'string' | ||
| ? prompt.id | ||
| : undefined; |
There was a problem hiding this comment.
When a config prompt carries prompt-level provider config, this branch only extracts the prompt's raw/id path and never traverses prompt.config. Promptfoo supports fields such as config.response_format on prompts, including file-backed schemas, so with config prompts enabled a PR that changes only schemas/response.json in prompts: [{ raw: '...', config: { response_format: 'file://schemas/response.json' }}] can still be skipped as unrelated.
Useful? React with 👍 / 👎.
| const assertionValues = Array.isArray(assert.value) | ||
| ? assert.value | ||
| : [assert.value]; |
There was a problem hiding this comment.
When an assertion puts file-backed data under config, such as a custom JS/Python assertion with config.fixture: file://fixtures/rubric.json, Promptfoo's config loader recursively resolves that file before evaluation, but this traversal only builds assertionValues from assert.value and never visits assert.config. With prompt filtering enabled, a PR that changes only that fixture can still be skipped even though the assertion receives different config.
Useful? React with 👍 / 👎.
Review + fix applied (
|
A directory dependency emitted without a trailing slash was only prefix-matched when isDirectory(dep) was true, so a PR that deletes the directory (isDirectory false) skipped the eval even though a watched child file changed. Always test the directory-prefix form; a file dependency has no children so it never false-matches. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Correctness fix pushed — deleted directory dependencyAn extended adversarial pass over the A directory dependency emitted without a trailing slash was only prefix-matched when |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7e61f115a
ℹ️ 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".
| const providers = | ||
| typeof config.providers === 'string' | ||
| ? [config.providers] | ||
| : config.providers; |
There was a problem hiding this comment.
When consumers invoke this action through uses:, action.yml runs dist/index.js, but this commit leaves dist/ byte-for-byte identical to its parent. The published action therefore still iterates a scalar provider string character-by-character and never executes this new normalization, so the fix only works in source-level tests. Run the packaging step and commit the regenerated dist/ artifacts.
AGENTS.md reference: AGENTS.md:L94-L99
Useful? React with 👍 / 👎.
Summary
Fixes Codex Security finding
csf_3ae84e911548184d0aa65a50(occ_e8f9f882fcec214787d09ed3).Promptfoo accepts scalar, array, object/map, templated, and script-backed configuration. The action now tracks the files the pinned Promptfoo runtime actually consumes so provider, prompt, test, validator, hook, transform, or fixture-only changes cannot incorrectly skip evaluation.
This patch:
use-config-promptsruns to ignore unused action globs.Verification
npm run all, post-artifact build/lint/test, bundle syntax/path-leak checks, artifactcmp, andgit diff --checkpass;distartifact from Check dist run29443256804(artifact8354334094, interim head624fe942ddd145c1c194428556d80e6890686ef4);fca13f896c61ad444efc2a661964ce7f514400b7.