fix: canonicalize provider file dependencies - #987
Conversation
|
@codex review Please review exact head |
There was a problem hiding this comment.
Pull request overview
This pull request fixes dependency extraction for Promptfoo provider references so the GitHub Action correctly detects provider-only changes (including Python file://...provider.py:function selectors and absolute provider paths) and doesn’t miss runs behind the “relevance” gate.
Changes:
- Canonicalizes provider
file://references by stripping only supported Python:functionselectors and handling string/object provider IDs consistently. - Preserves absolute provider paths (when inside the repo) for correct containment checks; improves Windows drive-colon handling for
file:///C:/...provider URLs. - Adds regression tests for provider canonicalization and updates the bundled
dist/index.js.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/utils/config.ts | Adds provider-specific file URL canonicalization and preserves absolute provider paths for correct dependency gating. |
| tests/utils/config.test.ts | Adds targeted regression coverage for Python selector stripping, absolute provider containment, invalid JS suffix behavior, Windows drive-colon handling, and explicit null configs. |
| tests/main.test.ts | Adds a main-gate regression to ensure provider file changes trigger the action run. |
| dist/index.js | Regenerates the bundled action output to include the updated dependency extraction logic (and updated bundled deps). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ovider-reference-parsing # Conflicts: # dist/index.js.map
Explain that only the Python `.py:function` selector is stripped (leaving an invalid `.js:callApi` untouched) and why the Windows drive-letter slash is dropped. Regenerate dist after merging main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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: 1210cfafbe
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80304634b2
ℹ️ 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: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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 |
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be587921c7
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67aba3522b
ℹ️ 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".
| } else if (mayRenderFileUrl(promptPath)) { | ||
| processPotentialFileUrl(promptPath); |
There was a problem hiding this comment.
When a prompt entry is an env template that renders to a normal relative prompt path, such as PROMPT_PATH=prompts/current/main.txt with prompts: ['{{ env.PROMPT_PATH }}'], this branch calls processPotentialFileUrl() and never falls through to the existing relative-path heuristic below. The extractor records no dependency, so a PR that only changes prompts/current/main.txt can be skipped even though Promptfoo evaluates that prompt file.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| const fileUrl = testPath.startsWith('file://') | ||
| ? testPath | ||
| : `file://${testPath}`; |
There was a problem hiding this comment.
When tests is an env template that renders to a file URL, for example TESTS=file://tests/cases.yaml with tests: '{{ env.TESTS }}', this check prefixes the raw template before rendering, so processFileUrl() sees file://file://tests/cases.yaml and records file:/tests/cases.yaml instead of tests/cases.yaml. A PR changing only the test file then misses the dependency gate; decide whether to add file:// from the rendered test path instead.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| ) { | ||
| return; | ||
| } | ||
| const sheetSeparator = value.indexOf('#'); |
There was a problem hiding this comment.
When a templated test path renders to an Excel sheet reference, such as TESTS=tests/cases.xlsx#Sheet1 with tests: '{{ env.TESTS }}', the sheet separator is searched on the raw template rather than the rendered path. The dependency is recorded as tests/cases.xlsx#Sheet1, while GitHub reports changes to tests/cases.xlsx, so a PR that only updates the spreadsheet can be skipped even though Promptfoo reads it.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| current as { options?: { provider?: unknown } } | ||
| ).options?.provider; | ||
| if (gradingProvider) { | ||
| processProviderValue(gradingProvider, true); |
There was a problem hiding this comment.
When an imported test file defines a grading provider under options.provider, this path is still resolved from the main config directory, unlike the test-level provider just above. For example, tests/cases.yaml containing options: { provider: 'python:graders/rubric.py:call_api' } makes Promptfoo load tests/graders/rubric.py, but the action watches graders/rubric.py, so a PR changing only the actual grading provider can be skipped.
Useful? React with 👍 / 👎.
mldangelo
left a comment
mldangelo
left a comment
There was a problem hiding this comment.
Canonicalizes provider file dependencies and conservatively resolves templated/mapped provider paths and env templates. This is the most defensively-bounded member of the family — explicit MAX_PROVIDER_VALUES (1024), MAX_PROVIDER_CONFIGS (128), MAX_GLOB_MATCHES (4096), and MAX_BRACE_EXPANSIONS caps, plus recursion guards (activeProviderObjects WeakSet, a visit counter) — which is good. But it overlaps almost entirely with #979 (scalar provider deps) and substantially with #974 (mapped providers): three separate PRs independently reworking provider dependency extraction on the same file. I did a structural pass here; the caps and guards look sound, and it keeps the fail-open return [] catch. The overriding recommendation is to pick one provider implementation and consolidate.
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 with #979 and #974 on provider dependency extraction specifically — those three should become one.
Review (current head
|
isValidFunctionName restricted .go selectors to call_api/CallApi, so file://p.go:OtherFunc recorded a dependency literally named 'p.go:OtherFunc' that never matches the real p.go, missing a re-eval when p.go changes. promptfoo's GolangProvider strips any :function selector for a .go file (like .py/.rb), so align .go with the generic function-name check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Correction: a real bug was here — fix pushed (
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9a80e968f
ℹ️ 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 (config.extensions) { | ||
| for (const extension of config.extensions) { | ||
| processFileUrl(extension, false, false, configEnvironment, false, true); |
There was a problem hiding this comment.
When an extension is configured as a plain file path with a hook selector, e.g. extensions: ['extensions/hooks.js:extensionHook'], this passes the raw string into the script-path parser, which blindly drops the first file://. Promptfoo documents extensions as file paths with a function name, so the action records ons/hooks.js instead of extensions/hooks.js; a PR changing only the actual hook file can miss the dependency gate and skip an evaluation that would run changed code.
Useful? React with 👍 / 👎.
| } else if (mayRenderFileUrl(promptPath)) { | ||
| processPotentialFileUrl(promptPath); |
There was a problem hiding this comment.
When a structured prompt is referenced in the common string/id form, such as prompts: ['file://prompts/chat.yaml'] or { id: 'file://prompts/chat.yaml' }, this branch only records the YAML/JSON file itself. Since Promptfoo's YAML/JSON prompt processors recursively resolve file:// references inside those files, a PR that changes only a nested fragment and no watched prompt glob/config file can leave dependencyChanged false and skip an evaluation that would load the changed fragment; inspectStructuredPrompt() currently only runs for object-form file: below.
Useful? React with 👍 / 👎.
| const executablePath = promptPath.slice('exec:'.length); | ||
| processFileUrl( | ||
| executablePath.startsWith('file://') | ||
| ? executablePath | ||
| : `file://${executablePath}`, |
There was a problem hiding this comment.
When an executable prompt includes an interpreter or arguments, such as prompts: ['exec:python scripts/build_prompt.py --mode ci'], this prefixes the whole command as a single file URL and records python scripts/build_prompt.py --mode ci instead of the script that Promptfoo executes after splitting the command. In workflows that set prompts for gating, a PR changing only scripts/build_prompt.py can therefore miss dependencyChanged and skip an evaluation that would run changed prompt-generation code.
Useful? React with 👍 / 👎.
| if (!/\.(?:ya?ml|jsonl?)$/i.test(externalTestPath)) { | ||
| continue; |
There was a problem hiding this comment.
When external tests are loaded from CSV, for example tests: file://cases.csv with a __expected value like file://validators/check.js, this extension gate records only the CSV file and skips inspecting the assertion dependencies inside it. Promptfoo supports CSV test files and parses file-backed JavaScript assertions from __expected, so a PR changing only the validator can leave dependencyChanged false and skip an evaluation that would load changed assertion code.
Useful? React with 👍 / 👎.
A directory dependency emitted without a trailing slash (e.g. 'tools') 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>
Another correctness fix pushed (
|
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: 712af1971f
ℹ️ 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".
| for (const provider of config.providers) { | ||
| if (typeof provider === 'string' && provider.startsWith('file://')) { | ||
| processFileUrl(provider); | ||
| processFileUrl(provider, true); |
There was a problem hiding this comment.
This source change is not included in dist/, which is byte-for-byte identical to the parent commit. Because action.yml:5 executes dist/index.js, users invoking the action still run the old processFileUrl implementation and function-qualified Python provider changes remain undetected; the rebuild check in .github/workflows/check-dist.yml:41-51 will also fail. Regenerate and commit the bundle as required.
AGENTS.md reference: AGENTS.md:L96-L99
Useful? React with 👍 / 👎.
Summary
Fixes Codex Security finding
csf_3ab060a90865e4376ff77143(occ_5a7cef563208654a40c215b3).Promptfoo supports Python provider references such as
file://providers/provider.py:custom_calland absolute local provider URLs. The action retained the function selector in the dependency filename and re-rooted absolute paths, so provider-only changes could miss the relevance gate.This patch:
.py:functionreferences;:callApisyntax un-reinterpreted;Other file-bearing config fields retain their existing resolution semantics.
Verification
provider.pyfiles;Could not identify provider;npm run allpasses: 9 test files, 244 tests, 100% coverage;npm run packageleaves the tree clean.Packaging note
Current
mainreferences a js-yaml 5.2.1 npm tarball that is no longer available, sonpm cicannot reproduce the merged dependency update. For bundle verification only, I built and packed the exact authoritative js-yaml Git tag (ac16b42c46c11c5c7f66062bfc78b168b5f07ecd) without changing repository metadata. The generated license artifact matchedmainbefore bundling this patch.