fix: track file-backed default tests - #982
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 GitHub Action by ensuring defaultTest dependencies are detected when defaultTest is specified as a file://... reference (not just an inline object), preventing relevant PRs from being incorrectly skipped.
Changes:
- Extend
PromptfooConfig.defaultTesttyping to allow either a string (includingfile://...) or an inline object. - Update dependency extraction to process file-backed
defaultTestvia the existing repository-containment-aware file visitor. - Add unit tests covering file-backed
defaultTest, repository-escape rejection, and ignoring unsupported strings; regeneratedist/bundle.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/utils/config.ts | Add support for defaultTest: file://... in dependency extraction and update config typing. |
| tests/utils/config.test.ts | Add coverage for file-backed defaultTest dependency extraction and containment rejection. |
| dist/index.js | Regenerated bundled output reflecting the source change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Swish! 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". |
…le-backed-default-test # Conflicts: # __tests__/utils/config.test.ts # dist/index.js.map
cfeabbb to
b4931a0
Compare
![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: b4931a0d43
ℹ️ 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 Please review exact head |
|
@codex review Please re-review exact head |
Review (current head
|
A dangling YAML list item (prompts:\n - file://a.txt\n -) parses to a null entry; the prompts loop then hit `typeof null === 'object' && null.file` and threw, which the outer catch turned into a ['./'] whole-repo watch, discarding the precise dependencies and disabling filtering. Skip null/undefined entries at the top of the loop, matching the tests/assert paths. 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: 770d9bf87d
ℹ️ 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.nunjucksFilters) { |
There was a problem hiding this comment.
When a prompt-gated workflow uses Promptfoo's documented modular form env: file://configs/env-prod.yaml (see https://www.promptfoo.dev/docs/configuration/modular-configs/#environment-specific-configurations), this traversal reaches the top-level passes for scenarios and Nunjucks filters without ever visiting config.env. A PR that changes only the env file can therefore leave dependencyChanged false and hit the skip path even though Promptfoo will load different environment values for the eval; add an env pass that records file-backed env configs before returning dependencies.
Useful? React with 👍 / 👎.
|
|
||
| // Process tests | ||
| if (config.tests) { | ||
| if (Array.isArray(config.tests)) { |
There was a problem hiding this comment.
When a config uses top-level assert entries, which the Promptfoo config reference supports, this code goes from defaultTest straight into tests and never walks config.assert. In prompt-gated runs, changing only a file used by a global assertion such as assert: [{ type: 'javascript', value: 'file://validators/global.js' }] can leave dependencyChanged false and skip evaluation even though Promptfoo will execute the changed assertion for the suite.
Useful? React with 👍 / 👎.
| /\.(?:json|ya?ml)$/i.test(fileUrl) || | ||
| getGlobMagic(fileUrl.slice('file://'.length)) === true | ||
| ) { | ||
| inspectNestedConfigFile(fileUrl); |
There was a problem hiding this comment.
When prompts points at a YAML prompt list and that file contains object-form prompts like - raw: exec:./generator.sh, Promptfoo supports executing that script, but this branch delegates the imported list to the generic config walker instead of the prompt-specific logic below that understands raw/exec: prompts. A PR changing only generator.sh can therefore be missed by dependency gating while the generated prompts used by the eval change; parse imported prompt config entries with the same prompt handling used for top-level prompt objects.
Useful? React with 👍 / 👎.
|
|
||
| // Process tests | ||
| if (config.tests) { | ||
| if (Array.isArray(config.tests)) { |
There was a problem hiding this comment.
When a suite-level assertScoringFunction: file://scoring.js is configured, the Promptfoo config reference says that file supplies the custom score combiner, but this traversal only handles assertScoringFunction inside tests and defaultTest. A PR that changes only scoring.js can therefore be skipped even though the same model outputs may now pass or fail differently; visit the top-level scoring function before moving on to tests.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| if (config.nunjucksFilters) { |
There was a problem hiding this comment.
When the config sets file-backed CLI defaults such as commandLineOptions: { promptPrefix: 'prefix.txt', envPath: '.env.evals' }, Promptfoo applies those defaults during eval, but this dependency scan never visits config.commandLineOptions. In prompt-gated runs, a change limited to the prefix or env file can leave dependencyChanged false and skip an evaluation whose rendered prompts or provider settings would change; add a bounded pass for file-valued command-line defaults.
Useful? React with 👍 / 👎.
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: 72ac19fa49
ℹ️ 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".
| extractAssertFiles(config.defaultTest.assert); | ||
| if (typeof config.defaultTest === 'string') { | ||
| if (config.defaultTest.startsWith('file://')) { | ||
| processFileUrl(config.defaultTest); |
There was a problem hiding this comment.
When defaultTest uses an absolute URL such as file:///home/runner/work/repo/defaults/default.yaml, processFileUrl() passes the absolute portion through path.join(configDir, filePath), which turns it into a path beneath the config directory (for example evals/home/runner/...) instead of preserving the absolute path. Consequently, a change to the actual default-test file is absent from the dependency set and a prompt-gated run can incorrectly skip evaluation; handle absolute paths before joining them to configDir.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72ac19fa49
ℹ️ 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".
| extractAssertFiles(config.defaultTest.assert); | ||
| if (typeof config.defaultTest === 'string') { | ||
| if (config.defaultTest.startsWith('file://')) { | ||
| processFileUrl(config.defaultTest); |
There was a problem hiding this comment.
When consumers invoke this action, action.yml executes dist/index.js, but this commit leaves that file byte-identical to its parent; the bundled defaultTest block still directly reads .vars and .assert, so the new string-handling branch is never executed by the distributed action. Regenerate and commit /dist/ with the source change.
AGENTS.md reference: AGENTS.md:L94-L98
Useful? React with 👍 / 👎.
| extractAssertFiles(config.defaultTest.assert); | ||
| if (typeof config.defaultTest === 'string') { | ||
| if (config.defaultTest.startsWith('file://')) { | ||
| processFileUrl(config.defaultTest); |
There was a problem hiding this comment.
When the configured default-test file is a repository symlink, such as defaults/current.yaml -> v2.yaml, this records only the lexical symlink path. A commit that changes defaults/v2.yaml without changing defaults/current.yaml therefore does not match the dependency list, even though Promptfoo follows the symlink and consumes the changed contents, so the action can skip the evaluation. Resolve the target, verify that its real path remains inside the workspace, and record that contained target as a dependency as well.
AGENTS.md reference: AGENTS.md:L56-L60
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2db2f0ce4
ℹ️ 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".
| extractAssertFiles(config.defaultTest.assert); | ||
| if (typeof config.defaultTest === 'string') { | ||
| if (config.defaultTest.startsWith('file://')) { | ||
| processFileUrl(config.defaultTest); |
There was a problem hiding this comment.
When a prompt-gated config uses a templated wrapper such as defaultTest: file://{{ env.DEFAULT_TEST_PATH }}, this new string branch passes the unresolved template to processFileUrl(). It is then recorded as a literal/glob path rather than the concrete runtime-selected file (and a root-level template may produce no dependency at all), so changing only the selected default-test file can incorrectly skip evaluation; dynamic wrapper paths should conservatively register the repository-root watcher.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99f693ff5b
ℹ️ 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 absolutePath = path.isAbsolute(filePath) | ||
| ? path.normalize(filePath) | ||
| : path.resolve(path.join(configDir, filePath)); |
There was a problem hiding this comment.
When an absolute file:// glob is used, such as an inline default-test var pointing to file:///workspace/repo/fixtures/*.txt, and a PR adds a new matching file, the glob is now expanded from the correct absolute path but the watcher calculation at lines 120-129 drops the leading root and rejoins the base beneath configDir. Because the newly added file is absent from the current expansion and the recorded base points somewhere like configDir/workspace/repo/fixtures, dependency gating can still skip the evaluation; derive the watcher from the absolute glob base rather than rejoining it to the config directory.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e7fc880dc
ℹ️ 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".
| extractAssertFiles(config.defaultTest.assert); | ||
| if (typeof config.defaultTest === 'string') { | ||
| if (config.defaultTest.startsWith('file://')) { | ||
| if (config.defaultTest.includes('{{')) { |
There was a problem hiding this comment.
When a file-backed default test selects its wrapper with a Nunjucks block, such as file://{% if env.CI %}defaults/ci.yaml{% else %}defaults/dev.yaml{% endif %}, Promptfoo renders the block but this condition recognizes only {{ expressions. The path therefore falls through to processFileUrl() as literal template text, so changing only the selected wrapper can fail to match any dependency and the prompt-gated action may skip evaluation; conservatively watch the dependency root for {% ... %} paths as well.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b10476d1a6
ℹ️ 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 (/\{[{%]/.test(config.defaultTest)) { | ||
| dependencies.add(`${dependencyRoot}${path.sep}`); | ||
| } else { | ||
| processFileUrl(config.defaultTest); |
There was a problem hiding this comment.
When defaultTest is a root-level glob such as file://*.yaml and a matching file is deleted, this call expands only files that remain, while processFileUrl() records no base watcher because the first path segment already contains glob magic. The deleted path is therefore absent from dependencies, so a prompt-gated run can incorrectly skip evaluation; record configDir when a glob has no literal prefix.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 582e2b7d8c
ℹ️ 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".
| extractAssertFiles(config.defaultTest.assert); | ||
| if (typeof config.defaultTest === 'string') { | ||
| if (config.defaultTest.startsWith('file://')) { | ||
| if (/\{[{%]/.test(config.defaultTest)) { |
There was a problem hiding this comment.
When a file-backed default-test path contains a Nunjucks comment, such as file://defaults/{# selected environment #}ci.yaml, Nunjucks removes the comment before loading defaults/ci.yaml, but this regex recognizes only {{ ... }} and {% ... %} tags. Fresh evidence beyond the prior block-tag case is the unhandled {# ... #} syntax: the scanner records the literal comment-bearing path, so changing only the rendered target can leave dependencyChanged false and incorrectly skip evaluation; treat comment tags as templated paths as well.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| const globPath = isAbsoluteGlob | ||
| ? path.relative(dependencyRoot, filePath) | ||
| : filePath; | ||
| const pathParts = globPath.split(/[\\/]/); |
There was a problem hiding this comment.
On POSIX runners, a repository directory may legitimately contain a backslash, and glob syntax represents that literal character with an escaped backslash (for example a pattern targeting fixtures\set/*.yaml). Splitting the pattern on both slash types rewrites that literal directory into fixtures/set when deriving the base watcher; existing matches are recorded individually, but adding a new matching file under fixtures\set/ will not match the incorrect watcher and can cause the action to skip evaluation. Split on the platform separator, or parse escaped glob characters without treating POSIX backslashes as directory separators.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 045f9dfbb6
ℹ️ 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 (/\{[{%#]/.test(config.defaultTest)) { | ||
| dependencies.add(`${dependencyRoot}${path.sep}`); | ||
| } else { | ||
| processFileUrl(config.defaultTest); |
There was a problem hiding this comment.
When defaultTest points to a YAML/JSON file whose contents reference another file (for example, vars.context: file://fixtures/context.txt), this branch records only the default-test wrapper and never loads it through extractVarFiles/extractAssertFiles. If only fixtures/context.txt changes, no dependency matches and the prompt-gated action can skip an evaluation even though Promptfoo consumes the changed file; inspect the file-backed default test and add its nested dependencies as well.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
Summary
Fixes Codex Security finding
csf_e8e50025fd2fff98d4dfaecc(occ_ae71e8315ee711bbd56a6b8c).Promptfoo accepts
defaultTestinline or throughfile://, and can recursively consume provider configs, schemas/tools, assertions, and external vars. The action previously missed several of those backing files and could incorrectly skip an evaluation when only a dependency changed.This patch:
previous_filename,--no-renames -z,trimWhitespace: false), treats the workspace-root dependency as the./full-eval sentinel, validates explicit prompt glob/match containment, canonically deduplicates overlapping absolute/relative prompt globs and emits repository-relative matches, preserves runtime-valid literal Ruby/Go vars colon filenames, anchors structured-prompt extension detection while retaining JSON/YAML glob inspection, scopes the CR/LF prompt guard to selected evaluation prompts before workflow-command sinks, avoids narrowing evaluation when a shared config/dependency changes with a prompt, and accurately and safely labels config/dependency/full-scan PR comments, workflow summaries, and manual logs;Check distbundle from artifact8354967307in run29444790386, built for source9df84aa77a86b6056719ff35fcc3ba7066702dd5and committed at final head3db0f7688d349d33b73a66742b7f6f08ac2a8f8e.Verification