fix: track scalar prompt dependencies - #977
Conversation
|
@codex review Please review the exact current head: |
There was a problem hiding this comment.
Pull request overview
This PR updates the action’s dependency-extraction logic to correctly handle Promptfoo configs where prompts is provided as a scalar string (e.g., prompts: file://prompt.txt), ensuring prompt-only changes are detected and evaluated instead of incorrectly taking the “no changes” path.
Changes:
- Extend the config model so
promptscan be either a scalar string or an array, and normalize scalars to an array before dependency extraction. - Add regression tests covering scalar file prompts, scalar glob prompts, and inline scalar prompts.
- Regenerate the bundled
dist/output to ship the fix.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/utils/config.ts |
Accepts scalar prompts and normalizes to an array before extracting dependencies. |
__tests__/utils/config.test.ts |
Adds coverage for scalar prompt dependency extraction (file, glob, inline). |
dist/index.js |
Updates the shipped bundle to include the new scalar prompt handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Keep it up! 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-prompt-dependencies # Conflicts: # dist/index.js.map
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: 84f2622786
ℹ️ 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".
…-pr-977-019f62c3 # Conflicts: # dist/index.js.map
|
@codex review |
|
@codex review |
main.ts duplicated config.ts's glob-magic options (with the derived brace limit hardcoded as 1025). Export GLOB_MAGIC_OPTIONS from config.ts and import it in main.ts so the dependency producer and matcher can't drift out of sync. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59ad02d759
ℹ️ 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".
| ) { | ||
| throw new Error('Config glob is malformed or exceeds supported limits'); | ||
| } | ||
| if (/[*?[\]{}()]/.test(configPattern)) { |
There was a problem hiding this comment.
When the configured config file is literally named with glob metacharacters, such as promptfoo[prod].yaml, this branch now treats the documented config path input as a glob. glob.sync will either fail to find the existing literal file or match a different file like promptfoop.yaml, so existing workflows with bracketed config filenames can fail before evaluation or run against the wrong config; only enter glob expansion when the input is intentionally a glob or preserve a literal-path fallback.
Useful? React with 👍 / 👎.
| extractTestValues(config.tests); | ||
| extractTestValues(config.scenarios); |
There was a problem hiding this comment.
Promptfoo supports suite-level assert and assertScoringFunction, but after handling defaultTest this only walks tests and scenarios. In configs that keep a shared validator at the top level, such as assert: [{ type: javascript, value: file://validators/check.js }], changing validators/check.js will not set dependencyChanged, so workflows with prompts globs can skip an evaluation even though Promptfoo would load the changed assertion file.
Useful? React with 👍 / 👎.
| !promptPrefixes.some((prefix) => | ||
| promptCandidate.startsWith(prefix), | ||
| ) |
There was a problem hiding this comment.
When the action prompt glob is written with a leading ./, such as ./prompts/*.txt, GitHub reports a deleted file as prompts/deleted.txt while promptPrefixes keeps ./prompts/. This prefix guard returns false before the matcher runs, so a PR that only deletes a watched prompt can still take the no-change skip path; normalize the expanded prefix the same way promptCandidate is normalized before comparing.
Useful? React with 👍 / 👎.
Review + fix applied (
|
…t path An inline prompt like "Explain the code in utils.py" (no separator, not a template) was classified as a file dependency via hasPromptFileExtension and the has-short-extension fallback, yielding a bogus dependency plus a './' whole-repo watch, so the eval re-ran on every push. Gate both on a new referenceCouldBePath (no whitespace, or has a path separator, or is a template), which preserves real paths with spaces and templated paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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: 8cc7fd6e50
ℹ️ 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 prompt of prompts) { | ||
| if (typeof prompt === 'string' && prompt.startsWith('file://')) { |
There was a problem hiding this comment.
When a supported scalar prompt is declared without the optional file:// prefix, such as prompts: prompts/main.txt, the new normalization reaches this loop but the following startsWith('file://') guard discards it. If only prompts/main.txt changes and the Action's own prompt glob does not match it, dependencyChanged remains false and the evaluation can be skipped even though Promptfoo loads that prompt file.
AGENTS.md reference: AGENTS.md:L5-L9
Useful? React with 👍 / 👎.
| for (const prompt of prompts) { | ||
| if (typeof prompt === 'string' && prompt.startsWith('file://')) { | ||
| processFileUrl(prompt); |
There was a problem hiding this comment.
For a scalar function prompt such as prompts: file://prompts/build.py:create_prompt, this passes the complete selector-bearing string to processFileUrl(), which records prompts/build.py:create_prompt rather than the actual prompts/build.py file. A PR changing only the Python or JavaScript module therefore cannot match the recorded dependency and may take the no-change skip path; remove the supported function selector before resolving the filesystem path.
AGENTS.md reference: AGENTS.md:L5-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: 69affddd1b
ℹ️ 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 = | ||
| typeof config.prompts === 'string' ? [config.prompts] : config.prompts; |
There was a problem hiding this comment.
When prompts uses Promptfoo's mapping form, for example prompts: { file://prompts/main.txt: Main }, this expression leaves the parsed object unchanged and the following for...of throws because the object is not iterable. The outer catch then returns an empty dependency list, discarding both the mapped prompt and dependencies already found in providers or tests, so a dependency-only change can incorrectly skip evaluation; convert mapping keys to prompt entries before iterating.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| typeof config.prompts === 'string' ? [config.prompts] : config.prompts; | ||
| for (const prompt of prompts) { | ||
| if (typeof prompt === 'string' && prompt.startsWith('file://')) { | ||
| processFileUrl(prompt); |
There was a problem hiding this comment.
When a scalar prompt uses an in-workspace absolute file URL such as prompts: file:///workspace/repo/prompts/main.txt, this passes it to processFileUrl, which strips the scheme and then combines the resulting absolute-looking path with configDir via path.join. Node consequently resolves a duplicated path such as /workspace/repo/workspace/repo/prompts/main.txt, so editing the actual prompt does not match the recorded dependency and the action can incorrectly skip evaluation; resolve absolute prompt paths directly before applying the config-directory base.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| typeof config.prompts === 'string' ? [config.prompts] : config.prompts; | ||
| for (const prompt of prompts) { | ||
| if (typeof prompt === 'string' && prompt.startsWith('file://')) { | ||
| processFileUrl(prompt); |
There was a problem hiding this comment.
When the scalar declaration is a root-level file glob such as prompts: file://*.txt, processFileUrl stops at the first path component because it is magical, leaving basePath empty and adding no directory dependency. If a matching root prompt is added or deleted, it is absent from the current glob matches and no root sentinel can match the changed filename, so an action prompt glob that does not independently cover it can take the no-change path; retain the config directory as the base watch when magic appears in the first component.
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: bbc61ec496
ℹ️ 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 ( | ||
| !/\s/.test(prompt) && | ||
| /\.(?:txt|md|json|ya?ml|py|[cm]?[jt]s|njk)$/i.test(prompt) | ||
| ) { |
There was a problem hiding this comment.
When a configuration uses an executable scalar prompt such as prompts: exec:./prompts/generate.sh, this branch neither recognizes the exec: prefix nor accepts the .sh suffix, so the script is absent from dependencies. In workflows whose action-level prompt glob does not include the script, a change to generate.sh can therefore take the no-change skip path even though Promptfoo executes that file; strip the exec: prefix and track the referenced executable.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| ) { | ||
| processFileUrl(`file://${prompt}`); | ||
| } | ||
| } else if (prompt.file) { |
There was a problem hiding this comment.
When a prompt object carries its backing reference in raw or id, for example { raw: 'exec:./prompts/generate.sh' } or { id: 'file://prompts/main.txt' }, this condition only considers prompt.file and silently ignores the referenced file. If the action-level prompt glob does not independently match that file, editing it will not set dependencyChanged and the evaluation can be skipped; process supported raw and id references 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: 01c9782aab
ℹ️ 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 (prompt.startsWith('exec:')) { | ||
| processFileUrl(`file://${prompt.slice('exec:'.length)}`); |
There was a problem hiding this comment.
When a scalar executable prompt includes normal command arguments or quoting, such as prompts: "exec:./scripts/generate.sh --tone formal", this passes the entire command remainder to processFileUrl(), recording a nonexistent filename containing the arguments rather than scripts/generate.sh. If the action-level prompt glob does not cover the script, changing only the script will not set dependencyChanged, so the evaluation can be skipped; extract the executable path from the command before tracking it.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| /\.(?:py|[cm]?[jt]s)$/i.test(promptPath.slice(0, selector)) | ||
| ? promptPath.slice(0, selector) | ||
| : promptPath; | ||
| processFileUrl(`file://${filePath}`); |
There was a problem hiding this comment.
When a scalar prompt points to a structured YAML or JSON prompt file that itself contains a runtime file reference, such as prompts: file://configs/prompts.yaml with content: file://prompts/system.txt, this records only the outer structured file and never inspects its contents. If only prompts/system.txt changes and the action-level prompt glob does not match it, the action can take the no-change path even though Promptfoo reloads that content; traverse supported file references in structured prompt files while guarding cycles.
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: a7073836e8
ℹ️ 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".
| .trimStart() | ||
| .match(/^(['"])(.*?)\1|^(\S+)/); | ||
| if (executable) { | ||
| processFileUrl(`file://${executable[2] ?? executable[3]}`); |
There was a problem hiding this comment.
When a scalar executable prompt uses an interpreter command such as prompts: 'exec:python scripts/generate.py', this parser selects only python and records it as the dependency, ignoring scripts/generate.py. If the action-level prompts glob does not include that script, changing it will not set dependencyChanged, so the action can take the no-change path even though Promptfoo executes the modified script.
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: cecc8a028d
ℹ️ 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".
|
|
||
| try { | ||
| const values: unknown[] = [ | ||
| loadYaml(fs.readFileSync(absolutePath, 'utf8'), { |
There was a problem hiding this comment.
When an untrusted checkout contains an in-workspace .yaml symlink whose target is outside the workspace, the lexical containment check passes and this new readFileSync dereferences the external target. If parsing fails, the catch forwards String(error) to the workflow log, and the bundled YAML parser includes source snippets in that error, potentially exposing target contents. Fresh evidence beyond the earlier direct-dependency symlink report is that this path actually reads and logs diagnostics from the target; resolve and validate the physical path before reading and avoid logging parser content.
AGENTS.md reference: AGENTS.md:L56-L60
Useful? React with 👍 / 👎.
| !absolutePath || | ||
| inspectedStructuredPrompts.has(absolutePath) || | ||
| !fs.existsSync(absolutePath) | ||
| ) { |
There was a problem hiding this comment.
When a scalar prompt is a structured glob such as file://prompts/*.yaml, processFileUrl expands and records the matches, but this check probes the unresolved glob path itself and returns without inspecting any matched YAML. Consequently, a change only to a nested dependency outside prompts/, such as shared/system.txt, can still take the no-change skip path. Fresh evidence beyond the earlier structured-scalar report is the combined glob case: traverse each safe glob.sync match rather than testing the literal pattern.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| const filePath = fileUrl.slice('file://'.length); | ||
| if (!/\.(?:json|ya?ml)$/i.test(filePath)) { | ||
| return; |
There was a problem hiding this comment.
When a scalar text prompt such as file://prompts/main.txt, main.md, or main.j2 contains a Nunjucks {% include %}, {% extends %}, or {% import %} directive, this extension guard returns without inspecting the template. A PR that changes only an imported partial outside the action-level prompt glob can therefore take the no-change skip path even though Promptfoo reloads that partial while rendering the prompt; inspect supported text templates or conservatively watch their dependency root when import directives are present.
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: bfd156471b
ℹ️ 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 contents = fs.readFileSync(physicalPath, 'utf8'); | ||
| if (isTextTemplate) { | ||
| for (const match of contents.matchAll( | ||
| /\{%-?\s*(?:include|extends|import|from)\s+(['"])(.*?)\1/g, |
There was a problem hiding this comment.
When a text prompt uses a valid dynamic Nunjucks import such as {% include partialName %}, this regex only recognizes directives whose argument begins with a quote and adds no conservative directory dependency for the unmatched directive. If partialName resolves to a partial outside the action-level prompt glob, changing that partial will not set dependencyChanged, so the evaluation can be skipped even though Promptfoo loads the modified template.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| if (typeof value === 'string' && value.startsWith('file://')) { | ||
| processPromptFile(value); |
There was a problem hiding this comment.
When a scalar YAML or JSON prompt contains a Nunjucks directive inside a prompt string, for example a YAML content block with {% include "shared/system.txt" %}, the structured traversal only processes strings beginning with file://. A change to the included partial can therefore miss dependencyChanged and take the no-change path when the action-level prompt glob does not cover that partial, even though the prompt renders it at evaluation time.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| /\.(?:py|[cm]?[jt]s)$/i.test(promptPath.slice(0, selector)) | ||
| ? promptPath.slice(0, selector) | ||
| : promptPath; | ||
| processPromptFile(`file://${filePath}`); |
There was a problem hiding this comment.
When use-config-prompts is false and a PR changes only this newly tracked scalar config prompt dependency, src/main.ts sets dependencyChanged but its promptFiles list contains only changed matches from the action-level prompt globs. Because that list is empty, it omits --prompts and Promptfoo evaluates the config's prompts instead of the action-input prompt suite; if a prompt and dependency both change, it similarly evaluates only the changed prompt rather than every prompt affected by the dependency.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| value && | ||
| typeof value === 'object' && | ||
| !inspectedValues.has(value) | ||
| ) { | ||
| inspectedValues.add(value); | ||
| values.push(...Object.values(value)); |
There was a problem hiding this comment.
When a structured YAML prompt contains binary metadata such as a large !!binary value, the YAML loader produces a typed byte array, but this branch treats it as a generic object and pushes every byte from Object.values onto values. A moderately sized embedded image can therefore create millions of queue entries and consume excessive memory and CPU during dependency extraction; recurse only through arrays and plain records, skipping typed arrays and other scalar object types.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efcd39ac9f
ℹ️ 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".
| .match(/^(['"])(.*?)\1|^(\S+)/); | ||
| if (executable) { | ||
| processPromptFile(`file://${executable[2] ?? executable[3]}`); |
There was a problem hiding this comment.
When use-config-prompts is enabled and an executable scalar prompt receives a repository file, such as exec:./scripts/generate.sh ./templates/input.txt, this branch processes only the first command token. A PR changing only templates/input.txt can therefore take the no-change path when the action-level prompt glob does not cover it, even though the executed prompt consumes the modified file; inspect path-like command arguments or conservatively watch their directory.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| inspectedPromptFiles.add(absolutePath); | ||
| const contents = fs.readFileSync(physicalPath, 'utf8'); |
There was a problem hiding this comment.
When a config references a large checked-in structured prompt, every PR with changed-file metadata reaches this unconditional whole-file read before the action decides whether any monitored file changed. The subsequent YAML parsing and object traversal can consume excessive memory and CPU—or crash the action—even for an unrelated README-only PR; verify that the target is a regular file and enforce a size/traversal budget before reading it.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
Summary
$reffragments.Security impact
Fixes Codex Security finding
csf_cedb9155ed2446aef69f420c/ occurrenceocc_740baac430d067e471c18aa5.Previously, supported dependencies could be omitted from change detection, allowing relevant changes to take the successful no-change path. The follow-up also closes path-escape, workflow-command, cyclic/oversized traversal, and adversarial-matching risks without exposing sensitive paths in diagnostics.
Validation
defaultTest, assertion-value arrays, nested$ref, and response-format schemas; remote structured datasets and Windows drive paths; HTTP computed discriminators, auth/TLS/signature/multipart and transforms plus structurally nested body fields; direct/default/options/assertion grading providers, Python/JavaScript transforms/rubrics/scoring, and extension hooks; provider-prefix/runtime selector compatibility; executable prompts/providers/targets, path-qualified interpreters, loaders/preloads, and arguments; in-checkout/external-config generic/HTTP/prompt/exec environment templates including leading-slash, comma-env, spaced/parenthesized/NBSP computed-env forms; NUL/whitespace/CRLF/rename/delete paths; workspace/symlink/dangling/EACCES containment; nonblocking FIFO/device, descriptor identity/TOCTOU, and bounded-growth reads; numeric/class-hidden/padded/parity/deep/comma/alphabetic brace, optimized[{]and escaped-literal classes, extglob, and traversal cases; Windows odd/even separator parity and linear normalization; runtime backslash-activated brace ranges/traversal with distinct raw action-input POSIX escape semantics; direct POSIX auth/TLS/signature/multipart/transform backslash-and-colon filenames, decoded multipart URLs, and supported file-URL shorthand; adversarial template/selector/spreadsheet lengths; unstructured and inline-HTTP Nunjucks include/extends/import/from after long and NBSP whitespace without widening ordinary HTTP bodies; deleted action-prompt glob matches with and without surviving siblings; bounded single-directory and recursive zero-match enumeration, cross-config/two-glob cumulative budgets, pre-descent symlink pruning, and canonical-directory retarget safety; compiled POSIX dependency matching at the PR-file cap; relative/deduplicated prompt selection, unused config-prompt isolation, and accurate PR/workflow reporting../behavior, actual Promptfoo nested-HTTP-body runtime parity, direct-read/glob parity, optimized/escaped-literal classes, pre-descent and canonical-directory containment, cumulative traversal budgets, Windows separator safety, and performance probes.8358320613from run29453153941(sha256:89b2f49cdf05dfeb9515ccd0562fdbb39d4c8ad2ffa16464b0fadaba12aec088) was verified from source6532032a21f5b1d82cdfdc4015db32156cab87eamerged into base77edad74c1051fcce344a1baaab27ce98c95acb7, compared byte-for-byte, and shipped in bundle commitee658a509889954416d95ff04a889dc6f3f49a51.