fix: lenient temporary ID validation — support underscores, warn instead of fail#24030
fix: lenient temporary ID validation — support underscores, warn instead of fail#24030
Conversation
- Update isTemporaryId regex to allow underscores (A-Za-z0-9_) so IDs like 'aw_pr_testfix' are now valid - Update TEMPORARY_ID_PATTERN and TEMPORARY_ID_CANDIDATE_PATTERN to include underscores for consistent matching - Change getOrGenerateTemporaryId to warn + auto-generate instead of returning an error when format is invalid - Change create_pull_request.cjs inline validation to warn + auto-generate instead of failing the whole PR creation - Update error messages throughout to mention underscore support - Update tests to reflect new behavior and new error messages Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8933463a-4aa4-485a-9e85-9ac4a4294ed4 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR makes temporary ID handling more permissive and resilient so that malformed temporary_id values no longer abort create_pull_request workflows, while also updating validation messaging and tests to reflect underscore support.
Changes:
- Expanded temporary ID validation to allow underscores (
A-Za-z0-9_) across helper regexes andisTemporaryId. - Updated
create_pull_requestandgetOrGenerateTemporaryIdto warn and fall back to an auto-generated ID instead of failing on invalid temp IDs. - Updated validation/error message text (and corresponding tests) for consistency.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/temporary_id.cjs | Allows underscores in temp IDs; warns + auto-generates on invalid input; updates validation messages. |
| actions/setup/js/temporary_id.test.cjs | Adds/updates tests for underscore support and the new warn+fallback behavior. |
| actions/setup/js/create_pull_request.cjs | Changes invalid temp ID handling from hard-fail to warning + auto-generated fallback. |
| actions/setup/js/update_project.cjs | Updates validation error messages to reflect underscore support. |
| actions/setup/js/update_project.test.cjs | Updates expectations to match the new validation message wording. |
Comments suppressed due to low confidence (2)
actions/setup/js/temporary_id.cjs:142
getOrGenerateTemporaryIdaccepts anentityTypeparameter (documented as being for error messages) but it’s never used. Either incorporateentityTypeinto the warning / error strings for clearer diagnostics, or remove the parameter to avoid dead API surface.
* @param {Object} message - The message object that may contain a temporary_id field
* @param {string} entityType - Type of entity (e.g., "issue", "discussion", "project") for error messages
* @returns {{temporaryId: string, error: null} | {temporaryId: null, error: string}} Result with temporaryId or error
*/
function getOrGenerateTemporaryId(message, entityType = "item") {
actions/setup/js/create_pull_request.cjs:272
- In the invalid temporary_id path,
temporaryIdis set togenerateTemporaryId()without the.toLowerCase()normalization applied in the valid-id path. For consistency with map lookups (which normalize to lowercase) and to keep returned IDs predictable, consider lowercasing (or usingnormalizeTemporaryId) on the auto-generated value too.
if (!isTemporaryId(normalized)) {
// Warn and auto-generate rather than failing - an invalid temporary_id is a minor issue
const autoGenerated = generateTemporaryId();
core.warning(
`Invalid temporary_id format: '${pullRequestItem.temporary_id}'. Temporary IDs must be in format 'aw_' followed by 3 to 12 alphanumeric or underscore characters (A-Za-z0-9_). Example: 'aw_abc' or 'aw_pr_fix'. Using auto-generated ID: '${autoGenerated}'`
);
temporaryId = autoGenerated;
} else {
temporaryId = normalized.toLowerCase();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
actions/setup/js/temporary_id.cjs
Outdated
| * Regex pattern for detecting candidate #aw_ references (any length, word-boundary delimited) | ||
| * Used to identify malformed temporary ID references that don't match TEMPORARY_ID_PATTERN | ||
| */ | ||
| const TEMPORARY_ID_CANDIDATE_PATTERN = /#aw_([A-Za-z0-9]+)\b/gi; | ||
| const TEMPORARY_ID_CANDIDATE_PATTERN = /#aw_([A-Za-z0-9_]+)\b/gi; | ||
|
|
There was a problem hiding this comment.
TEMPORARY_ID_CANDIDATE_PATTERN only matches [A-Za-z0-9_]+ up to a \b, so references that include invalid characters after a valid prefix (e.g. #aw_test-id) will be partially matched as #aw_test and won’t trigger the malformed-reference warning. Consider capturing the full token after #aw_ (up to whitespace/end/punctuation) and then validating the entire candidate so truly malformed IDs get warned about consistently.
| @@ -261,16 +261,15 @@ async function main(config = {}) { | |||
| const normalized = rawTemporaryId.startsWith("#") ? rawTemporaryId.substring(1).trim() : rawTemporaryId; | |||
|
|
|||
| if (!isTemporaryId(normalized)) { | |||
| // Warn and auto-generate rather than failing - an invalid temporary_id is a minor issue | |||
| const autoGenerated = generateTemporaryId(); | |||
| core.warning( | |||
| `Skipping create_pull_request: Invalid temporary_id format: '${pullRequestItem.temporary_id}'. Temporary IDs must be in format 'aw_' followed by 3 to 12 alphanumeric characters (A-Za-z0-9). Example: 'aw_abc' or 'aw_Test123'` | |||
| `Invalid temporary_id format: '${pullRequestItem.temporary_id}'. Temporary IDs must be in format 'aw_' followed by 3 to 12 alphanumeric or underscore characters (A-Za-z0-9_). Example: 'aw_abc' or 'aw_pr_fix'. Using auto-generated ID: '${autoGenerated}'` | |||
| ); | |||
| return { | |||
| success: false, | |||
| error: `Invalid temporary_id format: '${pullRequestItem.temporary_id}'. Temporary IDs must be in format 'aw_' followed by 3 to 12 alphanumeric characters (A-Za-z0-9). Example: 'aw_abc' or 'aw_Test123'`, | |||
| }; | |||
| temporaryId = autoGenerated; | |||
| } else { | |||
| temporaryId = normalized.toLowerCase(); | |||
| } | |||
There was a problem hiding this comment.
This file now has a second copy of temporary_id validation + fallback generation logic that overlaps with getOrGenerateTemporaryId in temporary_id.cjs. To avoid future drift (e.g., message wording / normalization differences), consider using the shared helper here and relying on it to warn + generate when invalid.
This issue also appears on line 263 of the same file.
|
@copilot review comments |
… shared helper in create_pull_request - TEMPORARY_ID_CANDIDATE_PATTERN now uses [A-Za-z0-9_-]+ (no \b) so #aw_test-id captures the full token 'test-id' instead of just 'test', triggering a warning as expected - create_pull_request.cjs now uses getOrGenerateTemporaryId() shared helper instead of duplicating the validation/fallback logic - Added test for hyphen-containing malformed temp ID reference Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a6bd2d2f-d14b-45b6-b5aa-61ea6b64360c Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both review comments in bf2a8a1:
|
An invalid
temporary_id(e.g.aw_pr_testfixcontaining an underscore) caused the entirecreate_pull_requestoperation to fail. Invalid temp IDs are minor issues and should not abort the workflow.Changes
isTemporaryIdregex from[A-Za-z0-9]{3,12}to[A-Za-z0-9_]{3,12}—aw_pr_testfixis now a valid IDgetOrGenerateTemporaryIdand the inline validation increate_pull_request.cjsnow emitcore.warning()and fall back to an auto-generated ID rather than returning{ success: false }TEMPORARY_ID_PATTERNandTEMPORARY_ID_CANDIDATE_PATTERNupdated to include_A-Za-z0-9_as the valid character set with examples likeaw_pr_fixupdate_project.cjs: Error messages updated for consistency (behavior unchanged — draft chaining references still reject truly malformed IDs)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:
https://api.github.com/repos/github/gh-aw/contents/.github%2Fworkflows%2Faudit-workflows.md/opt/hostedtoolcache/node/24.14.0/x64/bin/node /opt/hostedtoolcache/node/24.14.0/x64/bin/node --experimental-import-meta-resolve --require /home/REDACTED/work/gh-aw/gh-aw/actions/setup/js/node_modules/vitest/suppress-warnings.cjs --conditions node --conditions development /home/REDACTED/work/gh-aw/gh-aw/actions/setup/js/node_modules/vitest/dist/workers/forks.js D ode-gyp-bin/git git rev-�� --reverse origin/fallback-branch..HEAD node . git odules/npm/node_--count git(http block)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�� user.name Test User ode_modules/.bin/git . git ache/go/1.25.0/x-b git init�� --bare --initial-branch=main 0/x64/bin/git ranch test@test.com ode_modules/.binagent-change.txt git(dns block)If you need me to access, download, or install something from one of these locations, you can either: