Repository navigation
[jsweep] Clean update_work_item.cjs - #66772
github-actions[bot] wants to merge 3 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #66772 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines in business logic directories (threshold 100, no custom .design-gate.yml). Only 2 files changed.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
L15: delete: duplicate default-config handler type assertion. The preceding test already proves main() returns a handler; nothing replaces it.
net: -4 lines possible.
Generated by ✂️ Ponytail Reviewer for #66772 · codex · gpt56 · 7.9 AIC · ⌖ 6.36 AIC · ⊞ 13.4K
Comment /ponytail to run again
| it("returns a handler function", async () => { | ||
| expect(typeof (await main())).toBe("function"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
L15: delete: duplicate default-config handler type assertion. The preceding test already proves main() returns a handler; nothing replaces it.
There was a problem hiding this comment.
Removed the redundant default-config test; the remaining smoke test covers handler creation, and a focused assertion checks update-specific wiring. Included in 6fb14cf.
There was a problem hiding this comment.
Impeccable review (distill, extract modes) — refactor/cleanup change: JSDoc additions + a new, well-structured test file for update_work_item.cjs. Logic is unchanged.
No high-signal issues found:
- JSDoc is accurate and concise.
- New tests (6/6 passing) cover handler creation, default config, disabled-field rejection, area-path filtering, staged-mode no-op behavior, and input immutability — good coverage for a thin wrapper.
- No complexity to distill or reusable patterns to extract beyond what already exists in
azure_devops_work_items.cjs.
Approving.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 17.7 AIC · ⌖ 13 AIC · ⊞ 8.1K
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Blocking issues
The new wrapper test leaves global.fetch mocked across suite boundaries, and it repeats shared-handler assertions instead of focusing on wrapper-specific coverage.
Review themes
- Cross-suite test isolation is broken because
global.fetchis never restored. - The new cases largely duplicate
azure_devops_work_items.test.cjs, so maintenance cost increases without materially improving confidence inupdate_work_item.cjs.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 40.2 AIC · ⌖ 5.51 AIC · ⊞ 19.8K
Comment /review to run again
| describe("update_work_item", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| global.fetch = vi.fn(); |
There was a problem hiding this comment.
global.fetch is installed in beforeEach but never restored, so this file can leak a mocked network layer into later suites and make unrelated tests pass or fail for the wrong reason.
💡 Restore the global after each test
The existing shared-handler suite already deletes global.fetch in afterEach, and this wrapper test needs the same isolation. Without that cleanup, any later test file that forgets to stub fetch will silently run against this leftover mock instead of the real environment.
afterEach(() => {
delete global.fetch;
});That keeps this file self-contained and avoids cross-suite state pollution in CI.
There was a problem hiding this comment.
Added afterEach cleanup to delete the mocked global.fetch after every test. Included in 6fb14cf.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — small, non-blocking suggestions only.
📋 Key Themes & Highlights
Key Themes
- JSDoc consistency: the new JSDoc on
update_work_item.cjsdoesn't use the establishedHandlerFactoryFunctiontypedef that ~45 other safe-output handlers already share — worth aligning before this pattern gets copy-pasted into siblingado_*wrappers. - Test overlap: the new
update_work_item.test.cjssuite duplicates validation scenarios already covered byazure_devops_work_items.test.cjsrather than focusing on the wrapper's unique responsibility (type-string delegation).
Positive Highlights
- ✅ Logic is genuinely unchanged — this is a safe, low-risk documentation/spacing cleanup.
- ✅ New tests correctly verify staged-mode no-API-calls and input-immutability, both good regression guards for a handler that mutates fields in place elsewhere in the module.
- ✅ No behavior regressions found in the diff.
Note: pr-triage agent ran successfully and classified this as tests_only change — no fallback heuristic needed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 59.7 AIC · ⌖ 14.6 AIC · ⊞ 10.1K
Comment /matt to run again
| */ | ||
| async function main(config = {}) { | ||
| return createAzureDevOpsWorkItemHandler("ado_update_work_item", config); | ||
| } |
There was a problem hiding this comment.
[/codebase-design] New JSDoc uses ad-hoc @param {Object} / @returns {Promise<Function>} instead of the established @type {HandlerFactoryFunction} typedef that ~45 other safe-output handlers (e.g. noop_handler.cjs, add_comment.cjs) already use for this exact factory shape.
💡 Suggested fix
// @ts-check
const { createAzureDevOpsWorkItemHandler } = require("./azure_devops_work_items.cjs");
/**
* @typedef {import('./types/handler-factory').HandlerFactoryFunction} HandlerFactoryFunction
*/
/**
* Creates the Azure DevOps update work item safe-output handler.
* @type {HandlerFactoryFunction}
*/
async function main(config = {}) {
return createAzureDevOpsWorkItemHandler("ado_update_work_item", config);
}
module.exports = { main };Since this is the first ado_* wrapper to gain JSDoc, this pattern risks being copy-pasted into the other four sibling wrappers (create_work_item.cjs, assign_work_item.cjs, comment_on_work_item.cjs, link_work_items.cjs) by future jsweep runs, diverging from the codebase's shared vocabulary for handler factories and losing the stronger typing (HandlerConfig, MessageHandlerFunction) that the typedef provides.
@copilot please address this.
There was a problem hiding this comment.
Updated the wrapper JSDoc to use the shared HandlerFactoryFunction typedef. Included in 6fb14cf.
| it("works with default config", async () => { | ||
| const handler = await main(undefined); | ||
| expect(typeof handler).toBe("function"); | ||
| }); |
There was a problem hiding this comment.
[/tdd] Good coverage of the wrapper's thin-pass-through contract, but these 6 tests duplicate behavior already exercised in azure_devops_work_items.test.cjs (field rejection, area-path rejection use near-identical fixtures) without adding tests for behavior unique to this wrapper, e.g. that main() correctly forwards the "ado_update_work_item" type string vs. another type.
💡 Suggestion
Consider either: (a) trimming this file to just the handler-creation/default-config smoke tests (2 tests) since the validation logic is already fully covered in azure_devops_work_items.test.cjs, or (b) adding one assertion that proves type-specific wiring, e.g. asserting the error message explicitly references ado_update_work_item (which test 3 already does) and is not reused verbatim from another handler's config. As-is, 4 of the 6 tests re-verify handleUpdateWorkItem internals rather than the main() wrapper's own responsibility (delegating to the correct handler type), so a bug in the wrapper's type string wouldn't be caught by tests that only check validation errors.
@copilot please address this.
There was a problem hiding this comment.
Trimmed duplicated shared-handler cases and focused wrapper coverage on its configured handler type and input immutability. Included in 6fb14cf.
There was a problem hiding this comment.
🟡 Changes recommended
The input-mutation test does not exercise any code path that mutates the working message.
1 open finding
What changed in this PR
Adds documentation and targeted tests for the Azure DevOps work-item update wrapper.
Changes:
- Documents the handler factory.
- Adds six wrapper-focused tests.
| File | Description |
|---|---|
actions/setup/js/update_work_item.cjs |
Adds JSDoc without changing behavior. |
actions/setup/js/update_work_item.test.cjs |
Adds handler and validation tests. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| const handler = await main({ staged: true, target: "*", title: true }); | ||
| const message = { id: 42, title: "T" }; | ||
| await handler(message, {}); | ||
| expect(message).toEqual({ id: 42, title: "T" }); |
There was a problem hiding this comment.
Updated the regression test to pass body with a configured footer and verify the handler succeeds without mutating the caller's message. Included in 6fb14cf.
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: f32621f
|
…pdate-work-item-09ffbf59d471c611 Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
jsweep:
update_work_item.cjsContext: Node.js / github-script safe-output handler (thin wrapper over
azure_devops_work_items.cjs).Changes
@ts-checkwas already enabled (no@ts-nocheckto remove). Logic unchanged.update_work_item.test.cjswith 6 tests (handler creation, default config, disabled-field rejection, area-path rejection, staged mode makes no API calls, input message not mutated).Validation
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓azure_devops_work_items) ✓ 17/17