Fix JSDoc return type for upload artifact handler#25929
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/6a411b70-cabd-4ec6-b271-fefe8cba4a7b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/6a411b70-cabd-4ec6-b271-fefe8cba4a7b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a TypeScript typecheck failure by aligning the documented return type of the upload-artifact handler with the properties it actually returns.
Changes:
- Update
handleUploadArtifactJSDoc@returnstype to includeartifactIdandartifactUrl. - Change the runner for the
validate_workflowsjob fromubuntu-slimtoubuntu-latest.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/upload_artifact.cjs |
Expands JSDoc return type to include artifactId/artifactUrl to satisfy typechecking. |
.github/workflows/agentics-maintenance.yml |
Alters runner label for validate_workflows, changing the job execution environment. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| * @param {Object} resolvedTemporaryIds - Map of already-resolved temporary IDs (unused here) | ||
| * @param {Map<string, any>} temporaryIdMap - Shared temp-ID map; the handler does not modify it | ||
| * @returns {Promise<{success: boolean, error?: string, skipped?: boolean, tmpId?: string, artifactName?: string, slotIndex?: number}>} | ||
| * @returns {Promise<{success: boolean, error?: string, skipped?: boolean, tmpId?: string, artifactName?: string, artifactId?: number, artifactUrl?: string, slotIndex?: number}>} |
There was a problem hiding this comment.
The JSDoc type marks artifactUrl as optional, but handleUploadArtifact always returns an artifactUrl property (it may be an empty string). Consider making this artifactUrl: string (non-optional) to keep the annotation accurate and preserve type-safety for callers; keep artifactId optional since it can be undefined in staged mode / when the upload API doesn't return an id.
| * @returns {Promise<{success: boolean, error?: string, skipped?: boolean, tmpId?: string, artifactName?: string, artifactId?: number, artifactUrl?: string, slotIndex?: number}>} | |
| * @returns {Promise<{success: boolean, error?: string, skipped?: boolean, tmpId?: string, artifactName?: string, artifactId?: number, artifactUrl: string, slotIndex?: number}>} |
| validate_workflows: | ||
| if: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.operation == 'validate' && !github.event.repository.fork }} | ||
| runs-on: ubuntu-slim | ||
| runs-on: ubuntu-latest | ||
| permissions: |
There was a problem hiding this comment.
This PR is described as a JSDoc return-type fix, but it also changes the runner label for validate_workflows from ubuntu-slim to ubuntu-latest. The rest of this workflow (and many others in the repo) consistently use ubuntu-slim, so this looks like an unrelated change that could alter the execution environment and permissions assumptions. Please either revert this change or justify it in the PR description (or split into a separate PR) and ensure ubuntu-latest is intended here.
TypeScript typecheck fails because
handleUploadArtifactreturnsartifactIdandartifactUrlproperties not declared in its@returnsJSDoc annotation.artifactId?: numberandartifactUrl?: stringto the@returnstype onupload_artifact.cjs