fix: track file-backed tests - #981
Conversation
![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: 55f622e1ed
ℹ️ 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 review exact head |
There was a problem hiding this comment.
Pull request overview
Fixes an action dependency-tracking gap where Promptfoo test suites provided as file-backed paths (scalar/array/object { path } forms) weren’t being included in the action’s “changed files” inventory—causing test-file-only PRs to be treated as irrelevant and skipped.
Changes:
- Expanded
PromptfooConfig.teststyping and dependency extraction to support scalar, array, and{ path }test sources (includingfile://, bare paths, globs, function-qualified generators, and workbook sheet references). - Added regression tests covering the newly supported file-backed test forms and safety controls.
- Regenerated the bundled
dist/index.js.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/utils/config.ts | Adds normalization/parsing of file-backed test sources and tracks them as dependencies. |
| tests/utils/config.test.ts | Adds regression tests for scalar/array/object file-backed test suites and safety behavior. |
| dist/index.js | Updates the bundled action output to reflect the source changes. |
Comments suppressed due to low confidence (1)
tests/utils/config.test.ts:6
- This test file imports
@actions/coreand assertscore.warningcall counts, but it never callsvi.mock('@actions/core'). As a result,core.warningwill be the real function (not a mock/spyon), andtoHaveBeenCalledTimeswill fail or the test will emit real warnings. Add an explicit mock so Vitest uses__mocks__/@actions/core.ts.
import * as core from '@actions/core';
import * as fs from 'fs';
import * as glob from 'glob';
import type { Mock } from 'vitest';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { extractFileDependencies } from '../../src/utils/config';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55f622e1ed
ℹ️ 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".
…le-backed-tests # Conflicts: # dist/index.js.map
Add comments explaining the remote-source skip, sheet-reference stripping, and the drive-letter-safe function-qualifier guard in processTestFile. Regenerate dist after merging main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85fc1b2fcd
ℹ️ 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".
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
@codex review Please review exact head |
Review (current head
|
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: 16b2cf7993
ℹ️ 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".
| filePath = filePath.slice(0, functionIndex); | ||
| } | ||
|
|
||
| processFilePath(filePath, 'test file dependency'); |
There was a problem hiding this comment.
When tests uses an absolute local URL such as file:///repo/cases.yaml, this passes /repo/cases.yaml to processFilePath, but resolveConfigDependency constructs it with path.join(configDir, filePath), which rebases the absolute path to something like /repo/evals/repo/cases.yaml. The action therefore watches a nonexistent path and can skip an evaluation when the actual absolute test file changes; resolve absolute test paths without prepending configDir.
Useful? React with 👍 / 👎.
| const sheetIndex = filePath.indexOf('#'); | ||
| if (sheetIndex !== -1) { | ||
| filePath = filePath.slice(0, sheetIndex); |
There was a problem hiding this comment.
When a valid local test path contains # outside an Excel sheet qualifier, such as file://tests/cases#prod.yaml or file://tests#prod/cases.xlsx#Safety, this truncates the path at the first hash and records the wrong dependency. Promptfoo loads the literal hash-containing file or directory, so changing it can be classified as irrelevant; only strip a sheet suffix from the basename when the portion before it is an Excel filename.
Useful? React with 👍 / 👎.
| const functionIndex = filePath.lastIndexOf(':'); | ||
| if (functionIndex > 1) { | ||
| filePath = filePath.slice(0, functionIndex); |
There was a problem hiding this comment.
On POSIX, a non-generator test filename may legally contain a colon, for example tests: file://tests/cases:prod.yaml, but this unconditionally treats the last colon as a function selector and watches tests/cases instead. Promptfoo uses the actual cases:prod.yaml file, so edits to it can still be skipped; limit selector stripping to supported script-generator extensions.
Useful? React with 👍 / 👎.
| if (typeof test === 'string') { | ||
| processTestFile(test); | ||
| continue; |
There was a problem hiding this comment.
When a directly referenced YAML or JSON test file contains its own local dependencies, for example tests/cases.yaml with vars.context: file://data/context.txt or a file-backed assertion, this call records only tests/cases.yaml and never reads the test file. A PR that changes only the nested data or validator can therefore be skipped even though Promptfoo loads it while evaluating; inspect supported structured test files and add their nested file references to the dependency set.
Useful? React with 👍 / 👎.
| if (typeof test === 'string') { | ||
| processTestFile(test); | ||
| continue; |
There was a problem hiding this comment.
When a PR changes both one prompt file and a newly tracked file-backed test, this makes dependencyChanged true but leaves promptFiles containing the changed prompt; src/main.ts still appends --prompts whenever that list is nonempty, so Promptfoo evaluates only that prompt. Because the changed test applies to every configured prompt, results for unchanged prompt files are silently omitted; dependency changes should suppress the changed-prompt override and run the full config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25e1ecca4c
ℹ️ 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".
| ); | ||
| return repositoryFile !== configRepositoryPath; | ||
| }); | ||
| allPromptFiles.push(...matchingPrompts); |
There was a problem hiding this comment.
When multiple configured glob patterns match the same prompt, such as prompts/*.txt and prompts/shared.*, this appends that path once per pattern. A config or shared-test change then copies the duplicated list into promptFiles, so Promptfoo receives duplicate --prompts arguments and the PR comment or workflow summary repeats the file. Accumulate matches in a Set before constructing the evaluation list.
Useful? React with 👍 / 👎.
| if (configChanged || dependencyChanged) { | ||
| promptFiles.splice(0, promptFiles.length, ...allPromptFiles); | ||
| } |
There was a problem hiding this comment.
When only the config or a shared test dependency changes, this replaces promptFiles with every matched prompt even though none of those prompt files changed. The downstream PR comment uses this array in LLM prompt was modified in these files, so dependency-only PRs falsely claim that every prompt was modified; keep changed and evaluated prompt lists separate or change the reporting language.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 264c2cc004
ℹ️ 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 (configChanged || dependencyChanged) { | ||
| promptFiles.splice(0, promptFiles.length, ...allPromptFiles); |
There was a problem hiding this comment.
When the prompts input contains a traversal or absolute glob such as ../private/*.txt, allPromptFiles can contain files outside the checkout, and a config or dependency change now copies every match into promptFiles for Promptfoo's --prompts argument. This can cause local runner files to be read and submitted to the configured model provider; validate both the lexical and real resolved paths against workspaceRoot before evaluating these matches.
AGENTS.md reference: AGENTS.md:L56-L60
Useful? React with 👍 / 👎.
| if (config.tests) { | ||
| for (const test of config.tests) { | ||
| const tests = Array.isArray(config.tests) ? config.tests : [config.tests]; |
There was a problem hiding this comment.
When a Promptfoo config places a file-backed test under scenarios[].tests, such as scenarios: [{ tests: 'scenarios/cases.yaml' }], this block only processes top-level config.tests and records no dependency for the scenario test source. A PR that changes only scenarios/cases.yaml can therefore be classified as irrelevant and skip the evaluation even though Promptfoo loads that file.
Useful? React with 👍 / 👎.
|
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb46f79e64
ℹ️ 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 matchingPrompts = matches.filter((file) => { | ||
| const absolutePromptPath = path.resolve(workingDirectory, file); | ||
| if ( | ||
| !isPathInside(workspaceRoot, absolutePromptPath) || | ||
| !isPathInside(realWorkspaceRoot, fs.realpathSync(absolutePromptPath)) | ||
| ) { |
There was a problem hiding this comment.
When changed-file information contains no matching prompt, config, or dependency change, this still resolves and validates every glob match before reaching the skip check. Consequently, an unchanged prompt symlink that resolves outside the checkout fails every unrelated PR with INVALID_CONFIGURATION, even though that prompt would not be evaluated; select the evaluation set first and apply containment checks only to that set.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
| for (const scenario of config.scenarios ?? []) { | ||
| extractTests(scenario.tests); |
There was a problem hiding this comment.
When a supported file-backed scenario is configured as scenarios: [scenarios/shared.yaml], each scenario here is a string, so scenario.tests is undefined and the scenario file is never recorded as a dependency. A PR changing only that shared scenario can therefore be classified as irrelevant and skipped; process string scenario entries as files before inspecting object-form scenarios.
Useful? React with 👍 / 👎.
| configChanged || dependencyChanged | ||
| ? 'LLM prompts were evaluated' | ||
| : 'LLM prompt was modified'; |
There was a problem hiding this comment.
When use-config-prompts is true and a config or dependency changes, promptFiles contains matches from the action's prompts globs, but command construction intentionally omits those paths and evaluates the config-defined prompts instead. This branch nevertheless reports the glob-matched files as evaluated, so configurations whose action globs differ from their config prompts produce misleading PR comments and workflow summaries; describe config-defined prompts instead in this mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c83e4ec2e8
ℹ️ 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 (typeof scenario === 'string') { | ||
| processTestFile(scenario); | ||
| } else { | ||
| extractTests(scenario.tests); |
There was a problem hiding this comment.
When a scenario defines shared settings under scenarios[].config, for example a scenario config whose vars.context is file://data/context.json, this loop inspects only scenario.tests and never visits the scenario config. Promptfoo applies those settings while evaluating the scenario, so a PR that changes only data/context.json leaves dependencyChanged false and can incorrectly skip the evaluation; extract file references from scenario.config as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c83e4ec2e8
ℹ️ 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".
| filePath = filePath.slice(0, functionIndex); | ||
| } | ||
|
|
||
| processFilePath(filePath, 'test file dependency'); |
There was a problem hiding this comment.
When a newly supported test source is a glob beneath a repository symlink, such as tests: file://tests/external/*.yaml where tests/external resolves outside the checkout, this route accepts each match based only on its lexical path. The action can therefore enumerate and record host files outside the workspace despite the containment check; resolve each match with realpath and reject unverifiable or escaping targets before adding it.
AGENTS.md reference: AGENTS.md:L56-L60
Useful? React with 👍 / 👎.
| const reportedFiles = useConfigPrompts | ||
| ? configPath | ||
| : promptFiles.join(', '); |
There was a problem hiding this comment.
When a config or shared dependency changes and the action prompt globs match thousands of files, promptFiles now contains every match and this join copies the entire list into the issue-comment body. Once the filenames push the body beyond GitHub's comment-size limit, createComment rejects the request after the evaluation has completed, so the action fails without posting results; truncate or summarize the evaluated-file list before constructing the comment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d3c44a9b4
ℹ️ 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".
| let physicalBasePath: string; | ||
| try { | ||
| physicalDependencyRoot = fs.realpathSync(dependencyRoot); | ||
| physicalBasePath = fs.realpathSync(absoluteBasePath); |
There was a problem hiding this comment.
When a PR deletes the entire static directory matched by a file-backed test glob such as tests: file://cases/*.yaml, cases/ no longer exists in the checkout, so this realpathSync throws and the extractor returns without recording either the glob or its root. The changed path cases/example.yaml then matches no dependency, allowing the action to skip the evaluation even though the configured test set changed; retain a lexically contained watch pattern/root when a nonexistent glob base cannot be physically resolved.
AGENTS.md reference: AGENTS.md:L56-L60
Useful? React with 👍 / 👎.
| if (typeof test === 'string') { | ||
| processTestFile(test); | ||
| continue; |
There was a problem hiding this comment.
When a configured file-backed test is renamed without updating the config, this records only the now-missing source path, while the inspected PR flow keeps only GitHub's destination filename and push/manual comparisons use git diff --name-only, which likewise reports only the destination for detected renames. For example, renaming tests/cases.yaml to archive/cases.yaml leaves the dependency as tests/cases.yaml, so the action can classify the broken test configuration as irrelevant and skip the evaluation; include previous_filename for PRs and both paths from name-status rename records for git comparisons.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57ecd44a73
ℹ️ 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".
| config.defaultTest.startsWith('file://') && | ||
| !/\{[{%#]/.test(config.defaultTest) |
There was a problem hiding this comment.
When defaultTest uses an environment template such as file://{{ env.DEFAULT_TEST_PATH }}, this condition deliberately records no dependency. If the variable resolves to defaults/test.yaml and a PR changes only that file, dependencyChanged remains false and src/main.ts can skip the evaluation even though Promptfoo loads the modified default test; record a conservative workspace dependency or the resolved path instead of dropping templated sources.
Useful? React with 👍 / 👎.
Summary
Fixes Codex Security finding
csf_f207aa76a9dbf70c64f54808(occ_164363a1279743d491ec078a).Promptfoo accepts file-backed and structured test, prompt, provider, target, scenario, filter, and default-test configuration forms. Previously, dependency-only changes could be classified as irrelevant and skip evaluation.
This patch:
targets/defaultTest, including executable and structured prompts, nested provider maps, inline/external test and scenario hooks, scenarios, Nunjucks filters, HTTP transforms/parsers/file-auth/multipart/TLS/signature assets, generators, workbooks, assertion sets, and transitive refs/vars;csv-parseandminimatchtodependencies, and commits the exact trusted Linux-generated action bundle.Verification
../secrets/*.pyprompt glob, and a 1,025-item numeric prompt range with a constant pre-exec error; compiled external-config smokes return both trusted watch roots for unresolved env templates/malformed JSONL, reject odd-backslash 4,096-item provider/test ranges in 5 ms, and reject the brace-traversal arm before enumeration;npm run build && npm run biome:check && npm testpasses: 9 files, 563 tests, 100% statements/branches/functions/lines; source diff checks and trusted-artifact comparisons pass;29444547813embeds the exact source hashes forsrc/main.ts,src/utils/config.ts, andsrc/utils/glob.ts, runtimecsv-parse, and the locked YAML parser/license. Final head:bc3adb61ace96524701e24d2d2c58548fc98f7c4.