Conversation
- Use nullish coalescing (??) instead of logical OR (||) for config defaults (allowed, blocked, max) for more precise null/undefined handling - Add staged mode test suite (2 new tests covering preview behavior) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Cleans up the assign_to_user safe-output handler defaults and expands test coverage for staged mode behavior.
Changes:
- Updated
assign_to_user.cjsconfig defaulting to use nullish coalescing (??) instead of||. - Added a staged mode test suite (2 tests) and imported
afterEachfor env cleanup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| actions/setup/js/assign_to_user.cjs | Uses ?? for config defaults (allowed, blocked, max). |
| actions/setup/js/assign_to_user.test.cjs | Adds staged-mode tests and per-suite env var cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| beforeEach(() => { | ||
| process.env.GH_AW_SAFE_OUTPUTS_STAGED = "true"; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| delete process.env.GH_AW_SAFE_OUTPUTS_STAGED; |
There was a problem hiding this comment.
The staged-mode tests mutate process.env.GH_AW_SAFE_OUTPUTS_STAGED and then delete it in afterEach(). If the env var had a previous value (e.g. set by another test file running in the same worker), deleting it won’t restore the original value and can cause cross-test flakiness. Align with other tests (e.g. add_reviewer.test.cjs) by capturing the original value in beforeEach and restoring it in afterEach/finally.
| beforeEach(() => { | |
| process.env.GH_AW_SAFE_OUTPUTS_STAGED = "true"; | |
| }); | |
| afterEach(() => { | |
| delete process.env.GH_AW_SAFE_OUTPUTS_STAGED; | |
| let originalGhAwSafeOutputsStaged; | |
| beforeEach(() => { | |
| originalGhAwSafeOutputsStaged = process.env.GH_AW_SAFE_OUTPUTS_STAGED; | |
| process.env.GH_AW_SAFE_OUTPUTS_STAGED = "true"; | |
| }); | |
| afterEach(() => { | |
| if (originalGhAwSafeOutputsStaged === undefined) { | |
| delete process.env.GH_AW_SAFE_OUTPUTS_STAGED; | |
| } else { | |
| process.env.GH_AW_SAFE_OUTPUTS_STAGED = originalGhAwSafeOutputsStaged; | |
| } |
| const allowedAssignees = config.allowed ?? []; | ||
| const blockedAssignees = config.blocked ?? []; | ||
| const maxCount = config.max ?? 10; |
There was a problem hiding this comment.
config.allowed ?? [] / config.blocked ?? [] will now preserve non-nullish falsy values (e.g. false) rather than falling back to an empty array; that can lead to runtime errors later when .length / .join() are used on these variables. Consider normalizing types explicitly (e.g. only accept arrays; otherwise default to []) so misconfigured handler configs fail gracefully instead of throwing.
See below for a potential fix:
* Normalize the max count configuration value to a non-negative finite number.
* Falls back to the provided default if the value is nullish or invalid.
* @param {unknown} value
* @param {number} defaultValue
* @returns {number}
*/
function normalizeMaxCount(value, defaultValue) {
if (value === undefined || value === null) {
return defaultValue;
}
const num = Number(value);
if (!Number.isFinite(num) || num < 0) {
return defaultValue;
}
return num;
}
/**
* Main handler factory for assign_to_user
* Returns a message handler function that processes individual assign_to_user messages
* @type {HandlerFactoryFunction}
*/
async function main(config = {}) {
// Extract configuration
const allowedAssignees = Array.isArray(config.allowed) ? config.allowed : [];
const blockedAssignees = Array.isArray(config.blocked) ? config.blocked : [];
const maxCount = normalizeMaxCount(config.max, 10);
Summary
Cleaned
actions/setup/js/assign_to_user.cjsas part of the daily jsweep unbloat process.Context type: github-script (uses
core,github,contextglobals)Changes
assign_to_user.cjs??) instead of logical OR (||) for config defaults (allowed,blocked,max) — semantically more precise: defaults apply only when values arenull/undefined, not when they're falsy (e.g.max: 0now correctly means "zero limit" rather than falling back to 10)assign_to_user.test.cjsafterEachto vitest imports (needed for cleanup)staged modetest suite with 2 new tests:unassign_firstis correctly reflected in staged mode preview infoTest Improvements
Validation ✅
All four checks passed:
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓ (file already had@ts-check)npm run test:js✓ — all 23assign_to_usertests passWarning
The following domain was blocked by the firewall during workflow execution:
proxy.golang.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.