Merged
Conversation
- Replace verbose ternary patterns with nullish coalescing (??) - Remove redundant intermediate variables and comments - Add messages_staged.test.cjs with 15 test cases covering: - Default title and description rendering - Custom template from GH_AW_SAFE_OUTPUT_MESSAGES config - Fallback behaviour when config key is absent - Fallback on invalid JSON config - Empty operation value - Unrecognised placeholder passthrough - Independent config key isolation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closed
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors staged-mode message rendering to reduce verbosity and adds a dedicated test suite for staged title/description behavior.
Changes:
- Simplified
getStagedTitle/getStagedDescriptionby inlining defaults with??. - Removed intermediate variables/comments from
messages_staged.cjs. - Added
messages_staged.test.cjswith focused unit coverage for defaults and configurable templates.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/messages_staged.cjs | Refactors staged title/description template selection and rendering. |
| actions/setup/js/messages_staged.test.cjs | Adds unit tests for staged message defaults, config overrides, and fallback behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/messages_staged.cjs:37
- Same issue as above for
stagedDescription: switching to??means empty-string (or other falsy, non-nullish) config values will be treated as valid templates and forwarded torenderTemplate, changing the fallback behavior and potentially throwing if the value isn’t a string. Consider preserving the prior truthy fallback or adding a type/non-empty-string guard before rendering.
return renderTemplate(messages?.stagedDescription ?? "📋 The following operations would be performed if staged mode was disabled:", templateContext);
- Files reviewed: 2/2 changed files
- Comments generated: 3
|
|
||
| // Use custom title if configured | ||
| return messages?.stagedTitle ? renderTemplate(messages.stagedTitle, templateContext) : renderTemplate(defaultTitle, templateContext); | ||
| return renderTemplate(messages?.stagedTitle ?? "## 🔍 Preview: {operation}", templateContext); |
There was a problem hiding this comment.
Using nullish coalescing here changes behavior compared to the previous truthy check: if messages.stagedTitle is an empty string (or false/0 from a misconfigured JSON), it will now be passed into renderTemplate instead of falling back to the default. That can lead to an empty title or even a runtime error if the value isn’t a string. If the intent is to preserve the previous fallback semantics, use a truthy check (e.g., ||) or explicitly validate that the configured value is a non-empty string before calling renderTemplate.
This issue also appears on line 37 of the same file.
Comment on lines
+53
to
+57
| it("substitutes camelCase operation key as well as snake_case", () => { | ||
| process.env.GH_AW_SAFE_OUTPUT_MESSAGES = JSON.stringify({ stagedTitle: "Op: {operation}" }); | ||
| const title = getStagedTitle({ operation: OPERATION }); | ||
| expect(title).toContain(OPERATION); | ||
| }); |
There was a problem hiding this comment.
This test name suggests it’s validating camelCase vs snake_case placeholder support, but the template only uses {operation} and the context only provides operation (which is identical in camelCase and snake_case). Consider either renaming the test to match what it asserts, or adjust it to actually cover a camelCase key (e.g., pass a camelCase field in ctx and assert {some_snake_case} substitution works).
Comment on lines
+35
to
+51
| it("uses custom template from config", () => { | ||
| process.env.GH_AW_SAFE_OUTPUT_MESSAGES = JSON.stringify({ stagedTitle: "Staging: {operation}" }); | ||
| const title = getStagedTitle({ operation: OPERATION }); | ||
| expect(title).toBe(`Staging: ${OPERATION}`); | ||
| }); | ||
|
|
||
| it("falls back to default when stagedTitle is absent from config", () => { | ||
| process.env.GH_AW_SAFE_OUTPUT_MESSAGES = JSON.stringify({ runSuccess: "other" }); | ||
| const title = getStagedTitle({ operation: OPERATION }); | ||
| expect(title).toBe(`## 🔍 Preview: ${OPERATION}`); | ||
| }); | ||
|
|
||
| it("falls back to default when config is invalid JSON", () => { | ||
| process.env.GH_AW_SAFE_OUTPUT_MESSAGES = "not-json"; | ||
| const title = getStagedTitle({ operation: OPERATION }); | ||
| expect(title).toBe(`## 🔍 Preview: ${OPERATION}`); | ||
| }); |
There was a problem hiding this comment.
The updated implementation in messages_staged.cjs now relies on ?? for template fallback, which behaves differently for empty-string (and other falsy, non-nullish) configured values. There isn’t currently a test that locks in the intended behavior when stagedTitle/stagedDescription is set to "" (or another falsy value) in GH_AW_SAFE_OUTPUT_MESSAGES. Adding an explicit test case for this would prevent accidental behavior changes and clarify whether empty strings should fall back or be treated as valid templates.
Contributor
Author
🧪 Test Quality Sentinel ReportTest Quality Score: 76/100
Test Classification DetailsView All 15 Tests
Flagged Items — For Awareness
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
✅ Test Quality Sentinel: 76/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). All 15 JavaScript tests verify behavioral contracts with good edge-case coverage (invalid JSON, absent config keys, empty strings, unknown placeholders).
Collaborator
|
@copilot review all comments |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2d47f5dd-4997-4261-8783-ba569fe355b3 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Addressed all actionable review comments in commit Updates included:
Validation run:
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cleaned
actions/setup/js/messages_staged.cjsand added a comprehensive test suite.Context type: github-script (uses
core.*globals viamessages_core.cjs)Changes to
messages_staged.cjsSimplified the two functions by replacing verbose ternary patterns with the nullish-coalescing operator (
??):Before:
After:
The same simplification was applied to
getStagedDescription. Redundant intermediate variables and inline comments that simply restated the code were also removed.No logic was changed.
New test file:
messages_staged.test.cjsAdded 15 test cases covering:
getStagedTitle– default renderinggetStagedTitle– custom configstagedTitlekey, falls back when key absent, falls back on bad JSON, unknown placeholders keptgetStagedDescription– default renderinggetStagedDescription– custom configstagedDescriptionkey, falls back when absent, falls back on bad JSON, custom with operationValidation ✅
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js -- --no-file-parallelism✓ (all 15 new tests pass, full suite green)Warning
The following domain was blocked by the firewall during workflow execution:
invalid.example.invalidTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.