refactor: centralize prompt template path construction with getPromptPath helper#28589
refactor: centralize prompt template path construction with getPromptPath helper#28589
Conversation
…Path helper
Add getPromptPath(name) to messages_core.cjs that prefers GH_AW_PROMPTS_DIR
when set, otherwise falls back to ${RUNNER_TEMP}/gh-aw/prompts. Migrate all
26 repeated path construction sites across 11 handler files to use the helper.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8c62a493-c53a-4b24-9427-e8e7e986c45f
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🧪 Test Quality Sentinel ReportTest Quality Score: 72/100
Test Classification DetailsView all 4 tests
Flagged Tests — Requires ReviewNo tests require mandatory changes. One minor improvement suggestion: 💡 Missing edge case: neither env var setFile: 💡 Test inflationFile: Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §24959955578
|
There was a problem hiding this comment.
Pull request overview
This PR centralizes prompt template path construction by introducing a getPromptPath(name) helper (with GH_AW_PROMPTS_DIR override support) and migrating existing handlers away from hard-coded ${RUNNER_TEMP}/gh-aw/prompts/... paths.
Changes:
- Added
getPromptPath(name)tomessages_core.cjsand unit tests for expected env-var precedence. - Updated multiple handlers to call
getPromptPath(...)instead of inlining${RUNNER_TEMP}/gh-aw/prompts/<name>. - Updated
firewall_blocked_domains.cjsto usegetPromptPathwhile keeping a source-tree fallback for local dev/tests, and updated mocks accordingly.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/messages_core.cjs | Adds getPromptPath helper and exports it for broad use. |
| actions/setup/js/messages_core.test.cjs | Adds unit tests for getPromptPath env-var precedence and resets related env vars between tests. |
| actions/setup/js/setup_threat_detection.cjs | Uses getPromptPath for the threat detection prompt template location. |
| actions/setup/js/push_to_pull_request_branch.cjs | Uses getPromptPath for the manifest-protection fallback template. |
| actions/setup/js/handle_noop_message.cjs | Uses getPromptPath for noop issue/comment templates. |
| actions/setup/js/handle_detection_runs.cjs | Uses getPromptPath for detection-runs issue/comment templates. |
| actions/setup/js/handle_agent_failure.cjs | Uses getPromptPath for multiple agent-failure context templates and issue/comment templates. |
| actions/setup/js/firewall_blocked_domains.cjs | Switches to getPromptPath for runtime prompts while preserving source-tree fallback behavior. |
| actions/setup/js/create_report_incomplete_issue.cjs | Uses getPromptPath for the report-incomplete issue template. |
| actions/setup/js/create_missing_tool_issue.cjs | Uses getPromptPath for the missing-tool issue template. |
| actions/setup/js/create_missing_data_issue.cjs | Uses getPromptPath for the missing-data issue template. |
| actions/setup/js/create_pull_request.cjs | Uses getPromptPath for multiple manifest-protection and permission-denied fallback templates. |
| actions/setup/js/checkout_pr_branch.cjs | Uses getPromptPath for the checkout failure step-summary template. |
| actions/setup/js/checkout_pr_branch.test.cjs | Updates the messages_core.cjs mock to include getPromptPath. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 14/14 changed files
- Comments generated: 2
| function getPromptPath(name) { | ||
| const promptsDir = process.env.GH_AW_PROMPTS_DIR || `${process.env.RUNNER_TEMP}/gh-aw/prompts`; | ||
| return `${promptsDir}/${name}`; | ||
| } |
There was a problem hiding this comment.
getPromptPath() can produce an invalid path when neither GH_AW_PROMPTS_DIR nor RUNNER_TEMP is set (e.g. "undefined/gh-aw/prompts/"). It also claims to return an absolute path but doesn’t enforce that. Consider validating that at least one base directory is configured (throwing a clear error), and build/normalize the path with path.join/path.resolve (also avoiding double slashes and platform separator issues).
| let resolvedTemplatePath = templatePath; | ||
| if (!resolvedTemplatePath) { | ||
| resolvedTemplatePath = process.env.RUNNER_TEMP ? `${process.env.RUNNER_TEMP}/gh-aw/prompts/firewall_blocked_domains.md` : path.join(__dirname, "../md/firewall_blocked_domains.md"); | ||
| resolvedTemplatePath = process.env.RUNNER_TEMP || process.env.GH_AW_PROMPTS_DIR ? getPromptPath("firewall_blocked_domains.md") : path.join(__dirname, "../md/firewall_blocked_domains.md"); |
There was a problem hiding this comment.
The ternary condition process.env.RUNNER_TEMP || process.env.GH_AW_PROMPTS_DIR ? ... : ... relies on operator precedence and is a bit hard to scan. Consider adding parentheses (e.g. (process.env.RUNNER_TEMP || process.env.GH_AW_PROMPTS_DIR) ? ...) or extracting a boolean like hasRuntimePromptsDir to make the intent unambiguous.
| resolvedTemplatePath = process.env.RUNNER_TEMP || process.env.GH_AW_PROMPTS_DIR ? getPromptPath("firewall_blocked_domains.md") : path.join(__dirname, "../md/firewall_blocked_domains.md"); | |
| resolvedTemplatePath = (process.env.RUNNER_TEMP || process.env.GH_AW_PROMPTS_DIR) ? getPromptPath("firewall_blocked_domains.md") : path.join(__dirname, "../md/firewall_blocked_domains.md"); |
|
@copilot review all comments |
- getPromptPath now throws a clear error when neither GH_AW_PROMPTS_DIR nor RUNNER_TEMP is set, preventing silent "undefined/gh-aw/prompts/..." paths - firewall_blocked_domains.cjs: extract hasRuntimePromptsDir boolean to make the ternary intent unambiguous (Prettier-friendly alternative to parentheses) - Update test fixtures for buildLockdownCheckFailedContext, buildStaleLockFileFailedContext, and buildTimeoutContext to set a dummy RUNNER_TEMP so the new guard doesn't fire - Add test case asserting the new throw behaviour Agent-Logs-Url: https://github.com/github/gh-aw/sessions/934f6270-5038-4893-a6da-b8c8a34ce8b6 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Both review comments addressed in 43b09bd:
Three test fixtures in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
26 occurrences of
${process.env.RUNNER_TEMP}/gh-aw/prompts/<name>were scattered across 11 handler files with no support forGH_AW_PROMPTS_DIRoverrides—meaning any change to runtime prompt location required edits in a dozen places.Changes
getPromptPath(name)inmessages_core.cjs— resolves prompt template paths, preferringGH_AW_PROMPTS_DIRwhen set, falling back to${RUNNER_TEMP}/gh-aw/promptshandle_agent_failure.cjs(11),create_pull_request.cjs(3),handle_detection_runs.cjs(2),handle_noop_message.cjs(2), and 6 single-occurrence handlersfirewall_blocked_domains.cjs— updated to usegetPromptPathwhile preserving its existing local dev fallback to__dirname/../md/when neither env var is setcheckout_pr_branch.test.cjs— updatedmessages_coremock to includegetPromptPathBefore / After
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
invalid.example.invalid/usr/lib/git-core/git-remote-https /usr/lib/git-core/git-remote-https origin https://invalid.example.invalid/nonexistent-repo.git git conf�� --local --get n-dir/git user.email test@example.com--git-dir=/tmp/bare-incremental-kL20BS rgo/bin/git git comm�� -m Initial commit k/gh-aw/gh-aw/actions/node_modules/.bin/git /tmp/bare-incremgit . ache/uv/0.11.7/xagent-change.txt git(dns block)/usr/lib/git-core/git-remote-https /usr/lib/git-core/git-remote-https origin https://invalid.example.invalid/nonexistent-repo.git git conf�� user.email ode_modules/vitest/suppress-warnings.cjs rigin/token-option-test git-upload-pack git git-upload-pack push 64/bin/git ode_modules/viteorigin bran�� -M ion-test.patch.diff.tmp 86_64/git origin/auth-cleagit --stdout e_modules/.bin/gagent-change.txt git(dns block)/usr/lib/git-core/git-remote-https /usr/lib/git-core/git-remote-https origin https://invalid.example.invalid/nonexistent-repo.git e/git init�� ndor/bin/git git ode_modules/.bin/git =receive test@example.com--git-dir=/tmp/bare-incremental-oiqJk8 /git(dns block)If you need me to access, download, or install something from one of these locations, you can either: