fix: support prompt map dependencies - #974
Conversation
|
@codex review Please review the exact current head: |
There was a problem hiding this comment.
Pull request overview
This PR updates the GitHub Action’s config dependency extraction to correctly handle Promptfoo’s mapping-form prompts and to fail closed (raise) on dependency-extraction errors, preventing provider-only changes from being incorrectly skipped.
Changes:
- Support Promptfoo
promptsas either an array or a mapping, and extractfile://...dependencies from mapping keys. - Change dependency extraction error handling from “warn + return []” to “throw”, so the action fails closed.
- Update tests and regenerate the bundled
dist/output.
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 mapping-form prompt dependency extraction and changes extraction failures to throw. |
| tests/utils/config.test.ts | Adds regression tests for prompt maps and verifies fail-closed behavior on invalid YAML/read failures. |
| tests/main.test.ts | Ensures the action fails (and does not execute promptfoo) when dependency extraction throws. |
| dist/index.js | Regenerated bundle reflecting the updated dependency extraction logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: ddec68dcc2
ℹ️ 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".
…ompt-map-dependencies # Conflicts: # dist/index.js.map
Both the array and mapping forms feed the same `extractPromptFile` visitor, so select the iterable once (entries vs keys) instead of duplicating the for-loop across an if/else. Behavior unchanged; regenerate dist. Merges origin/main (js-yaml v5 config loading) and regenerates dist to clear the merge conflict. 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: ff1413db79
ℹ️ 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: 8a8a1c046d
ℹ️ 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 |
Two missed-re-eval bugs in the dependencyChanged matcher: - A directory dependency without a trailing slash (e.g. 'data') 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 changed. Always test the directory-prefix form; a file dependency has no children so it never false-matches. - config.ts classifies a bracket character class (file[1].txt) as a glob via hasGlobCharacterClass, but main.ts gated Minimatch behind glob.hasMagic, which reports no magic for single-char classes -> a deleted match like file1.txt was missed. Mirror config.ts's classification. 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: 41ade6d4dc
ℹ️ 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".
| processFileUrl( | ||
| `file://${renderedFilterPath}`, | ||
| true, |
There was a problem hiding this comment.
When a configured filter uses the supported file://filters/normalize.js form, this prepends a second scheme and passes file://file://filters/normalize.js to processFileUrl. That resolves as a literal file:/... path rather than the real filter script, so a PR changing only the filter can miss dependencyChanged and skip the evaluation; only add file:// when the configured value does not already have it.
Useful? React with 👍 / 👎.
Two more correctness fixes 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: dddfda4793
ℹ️ 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 (absolutePath) { | ||
| dependencies.add(absolutePath); | ||
| } | ||
| if (Array.isArray(config.prompts)) { |
There was a problem hiding this comment.
For every installed GitHub Action run, action.yml executes dist/index.js, but this commit changes only src/utils/config.ts; the committed bundle still performs for (const prompt of config.prompts) and therefore retains the mapping-form failure this patch is intended to fix. Regenerate and commit dist/ so consumers execute this new array/map branch.
AGENTS.md reference: AGENTS.md:L96-L98
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| core.warning( | ||
| `Failed to extract dependencies from config: ${error instanceof Error ? error.message : String(error)}`, | ||
| ); | ||
| return []; | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| throw new Error(`Failed to extract dependencies from config: ${message}`); |
There was a problem hiding this comment.
When the action is given a supported .js/.ts Promptfoo config and the event supplies any changed-file list, dependency extraction attempts to parse the module source as YAML; typical export default { ... } syntax throws, and this new rethrow propagates to run() and marks the action failed before Promptfoo can load the config. Detect executable config extensions and conservatively run/watch all changes rather than treating their non-YAML syntax as an extraction failure.
Useful? React with 👍 / 👎.
| const extractPromptFile = (prompt: PromptEntry): void => { | ||
| if (typeof prompt === 'string' && prompt.startsWith('file://')) { | ||
| processFileUrl(prompt); |
There was a problem hiding this comment.
When a mapping-form prompt uses another Promptfoo-supported reference form, such as prompts/main.txt: main prompt or exec:./prompts/generate.sh: generated prompt, each key reaches this helper but is discarded because it does not start with file://. With changed-file filtering enabled, changing only that prompt or generator can therefore leave dependencyChanged false and cause the action to skip the evaluation; mapped keys need the same bare-path and executable-reference handling as Promptfoo.
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: dddfda4793
ℹ️ 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 Object.keys(config.prompts)) { | ||
| extractPromptFile(prompt); |
There was a problem hiding this comment.
action.yml executes dist/index.js, but this commit leaves that blob byte-for-byte unchanged; the bundled extractor still iterates config.prompts directly and catches extraction failures by returning []. Consumers therefore receive none of the new prompt-map or fail-closed behavior even though the source tests pass. Regenerate and commit dist/ with the source changes.
AGENTS.md reference: AGENTS.md:L94-L99
Useful? React with 👍 / 👎.
| for (const prompt of Object.keys(config.prompts)) { | ||
| extractPromptFile(prompt); |
There was a problem hiding this comment.
When a prompt map key uses a supported bare path or command form, such as prompts/main.txt: main or 'exec:./prompts/generate.sh': generated, extractPromptFile ignores it because string entries are accepted only when they begin with file://. A PR changing only the referenced prompt or generator can therefore leave dependencyChanged false and skip the evaluation; handle the same bare-path and executable forms that Promptfoo loads at runtime.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| const extractPromptFile = (prompt: PromptEntry): void => { | ||
| if (typeof prompt === 'string' && prompt.startsWith('file://')) { | ||
| processFileUrl(prompt); |
There was a problem hiding this comment.
When a map key is an absolute URL such as file:///workspace/repo/prompts/main.txt, the new branch sends it to processFileUrl, where removing the scheme leaves an absolute-looking path that is passed through path.join(configDir, filePath). Node joins that under the config directory, producing a nonexistent path such as evals/workspace/repo/prompts/main.txt instead of the real in-workspace file, so an edit to the actual prompt is not detected. Resolve contained absolute paths directly before recording the dependency.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| const extractPromptFile = (prompt: PromptEntry): void => { | ||
| if (typeof prompt === 'string' && prompt.startsWith('file://')) { | ||
| processFileUrl(prompt); |
There was a problem hiding this comment.
When a mapped prompt glob has no matches in the post-change checkout—most notably when a PR deletes its last matched file—this path can return no usable dependency. For file://prompts/*.txt, the helper records prompts without a directory marker, so after the directory disappears isDirectory is false and the deleted prompts/old.txt does not match; a root-level file://*.txt records no base at all. Preserve a directory sentinel for zero-match globs so deletions still trigger evaluation.
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: 97f0bb7663
ℹ️ 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 (prompt.startsWith('file://')) { | ||
| processFileUrl(prompt); |
There was a problem hiding this comment.
When a prompt-map key uses Promptfoo's supported function-backed form, such as file://prompts/build.py:create_prompt, this passes the selector through as part of the filename and records prompts/build.py:create_prompt instead of prompts/build.py. A change to the actual script therefore does not match the dependency, allowing the action to take the no-change path; strip the function selector before processing the file URL.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| } 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 quoted prompt-map key is a valid bare path containing spaces, such as 'prompt assets/my generator.py': generated, this whitespace check discards it even though Promptfoo loads that path. If only that generator changes, no dependency matches and the evaluation can be skipped; classify path-like keys without rejecting all embedded whitespace.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| if (prompt.startsWith('file://')) { | ||
| processFileUrl(prompt); |
There was a problem hiding this comment.
When a mapped prompt such as file://prompts/chat.yaml contains a message referencing file://partials/system.txt, Promptfoo loads both files, but this branch records only the outer YAML path and never inspects its structured contents. A PR that changes only the partial can therefore miss every dependency match and skip the evaluation; traverse file references inside mapped YAML and JSON prompts 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: e9dac6e637
ℹ️ 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".
| : promptPath; | ||
| processFileUrl(`file://${filePath}`); | ||
| } else if (prompt.startsWith('exec:')) { | ||
| processFileUrl(`file://${prompt.slice('exec:'.length)}`); |
There was a problem hiding this comment.
When a mapped executable prompt includes normal command arguments, such as 'exec:./prompts/generate.sh --tone formal': generated, this branch records prompts/generate.sh --tone formal as one filename rather than the script path. Consequently, a PR changing only prompts/generate.sh does not match the extracted dependency and the evaluation can be skipped. The fresh evidence after the earlier executable-prompt comment is that the new branch still passes the entire command tail directly to processFileUrl; parse the command and track its file-backed arguments instead.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| (!/\s/.test(prompt) || /[\\/]/.test(prompt)) && | ||
| /\.(?:txt|md|json|ya?ml|py|[cm]?[jt]s|njk)$/i.test(prompt) |
There was a problem hiding this comment.
For a supported bare mapped function prompt such as 'prompts/build.py:create_prompt': generated, the key reaches this extension check with the selector still attached, so it no longer ends in .py and no dependency is recorded. A change to prompts/build.py can therefore be treated as unrelated. The fresh evidence after the prior selector fix is that selector stripping is performed only in the file:// branch; apply it before classifying bare prompt 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: 5f3934009a
ℹ️ 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 = resolveConfigDependency( | ||
| filePath, | ||
| 'structured prompt dependency', | ||
| ); |
There was a problem hiding this comment.
Although direct structured paths are now walked, a mapped glob such as file://prompts/*.yaml is resolved here as one literal wildcard path, so the subsequent existence check returns without inspecting any matched YAML. If a matched prompt references file://shared/system.txt outside the glob's base directory and only that shared file changes, it is absent from the dependency set and changed-file filtering skips an evaluation that would load the changed content; pass every expanded structured match through the walker.
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: 4ee825fa19
ℹ️ 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 value === 'string' && value.startsWith('file://')) { | ||
| processPromptFile(value); |
There was a problem hiding this comment.
When a mapped YAML or JSON prompt contains a function-backed reference such as file://prompts/build.py:create_prompt, this recursive branch passes the selector through to processPromptFile, which records prompts/build.py:create_prompt rather than the actual script. A change to prompts/build.py therefore does not match the extracted dependency and the action can incorrectly skip evaluation. Fresh evidence after the earlier top-level selector fix is that nested structured values still bypass the stripping performed in extractPromptFile.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| } else if ( | ||
| (!/\s/.test(prompt) || /[\\/]/.test(prompt)) && | ||
| /\.(?:txt|md|json|ya?ml|py|[cm]?[jt]s|njk)$/i.test(prompt) |
There was a problem hiding this comment.
.j2 in bare mapped prompt detection
When a prompt map uses a bare Jinja template path such as prompts/main.j2, this extension allowlist ignores the key even though the newly added visitor explicitly recognizes .j2 as a text-template file. If only that template changes, no dependency matches and the action can skip the evaluation. Fresh evidence after the earlier bare-path fix is that .j2 remains omitted here while .njk and the other supported template extensions are accepted.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| if (isTextTemplate) { | ||
| for (const match of contents.matchAll( | ||
| /\{%-?\s*(?:include|extends|import|from)\s+(['"])(.*?)\1/g, | ||
| )) { | ||
| processPromptFile(`file://${match[2]}`); |
There was a problem hiding this comment.
When a mapped text template uses a Nunjucks expression such as {% include partialTemplate %} or an array of fallback templates, this regex recognizes only directly quoted filenames and silently records no dependency. In a changed-file-filtered run where the resolved partial is outside the monitored prompt glob, editing only that partial can therefore skip an evaluation even though Promptfoo loads the changed content at render time; unsupported dynamic file-loading tags need to trigger conservative dependency handling rather than being ignored.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| const filePath = fileUrl.slice('file://'.length); | ||
| const isStructuredPrompt = /\.(?:json|ya?ml)$/i.test(filePath); | ||
| const isTextTemplate = /\.(?:txt|md|j2|njk)$/i.test(filePath); | ||
| if (!isStructuredPrompt && !isTextTemplate) { | ||
| return; |
There was a problem hiding this comment.
When a mapped .j2 or .njk prompt statically imports a template with another conventional suffix, for example base.html, the import itself is recorded but this extension gate prevents that file from being inspected. If base.html then includes partials/header.html, the header never enters the dependency set, so changing only it can skip evaluation even though Nunjucks loads it transitively; files reached through a template-loading tag should continue through the template visitor regardless of their suffix.
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: b614fa9de8
ℹ️ 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 match of contents.matchAll( | ||
| /\{%-?\s*(?:include|extends|import|from)\s+([\s\S]+?)-?%\}/g, |
There was a problem hiding this comment.
When a mapped template contains an inert dynamic tag inside a Nunjucks comment or {% raw %} block—for example {# {% include partialTemplate %} #}—this regex still matches it and adds the repository-root sentinel. Any unrelated changed file then sets dependencyChanged, causing the action to evaluate every prompt and potentially spend model quota even though Nunjucks never loads that dependency; exclude comments and raw blocks before scanning import tags.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| const isTextTemplate = | ||
| importedTemplate || /\.(?:txt|md|j2|njk)$/i.test(filePath); | ||
| if (!isStructuredPrompt && !isTextTemplate) { |
There was a problem hiding this comment.
When a mapped text prompt uses another valid suffix, such as file://prompts/main.html, the outer file is recorded but this allowlist returns before scanning its Nunjucks content. If main.html includes shared/header.html, changing only that partial leaves it out of the dependency set and can incorrectly skip the evaluation; treat non-structured file-backed prompts as text templates rather than limiting top-level inspection to these four extensions.
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: 347e0759ca
ℹ️ 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".
| !/\.(?:cjs|cts|j2|js|jsonl?|md|mjs|mts|py|ts|txt|ya?ml)(?::[A-Za-z_]\w*)?$/i.test( | ||
| prompt, | ||
| ) |
There was a problem hiding this comment.
.njk in bare prompt path detection
When a prompt map or scalar prompt uses a bare Nunjucks path such as prompts/main.njk, this extension allowlist rejects it, so changing only that template can leave dependencyChanged false and skip the evaluation. Unlike the previously reported .j2 case, this revision accepts .j2 but still omits the equally supported .njk extension.
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: 751def08d4
ℹ️ 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.
On Windows runners, a canonical mapped glob such as file:///D:/a/repo/prompts/*.txt reaches this code as /D:/a/repo/prompts/*.txt; path.normalize treats that as a current-drive-root path (\D:\a\...) rather than the drive-absolute path D:\a\.... Glob expansion and the fallback watcher root therefore use malformed paths, so changing or deleting the matched prompt can leave dependencyChanged false and skip evaluation. Fresh evidence beyond the prior absolute-path report is that this revision handles POSIX roots but still does not normalize the /D:/... Windows file-URL form.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
Summary
dist/bundleSecurity impact
Fixes Codex Security finding
csf_f464f43c8d24989b741c6cb1/ occurrenceocc_9e5db0cfa90fdd59bfc02eb2.Previously, a valid prompt map caused the array-only walker to throw after provider extraction. The outer catch returned
[], erased the provider dependency, and allowed a provider-only pull request to take the normal successful no-change path.Scope
This PR addresses mapping-form prompts and the fail-open exception boundary. Scalar prompts/providers and external tests/defaults remain separately tracked findings.
Validation
UnifiedConfigSchemaaccepts the original fixtureproviders/provider.pynpm run allpasses: 9 test files, 239 tests, 100% coveragenpm run packageleaves the tree clean