Feature/GitHub artifacts#42
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughUpdates the summary input from a boolean toggle to a mode selector, adds an HTML artifact upload option, rewrites the PowerShell summary output to handle multiple modes, and wires the action to upload and link the HTML report artifact. Changesstep_summary modes and HTML artifact upload
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant actions/github-script
participant Upload-HtmlReportArtifact.js
participant Twirp API
participant signed_upload_url
actions/github-script->>Upload-HtmlReportArtifact.js: invoke({ core })
Upload-HtmlReportArtifact.js->>Upload-HtmlReportArtifact.js: resolve path and validate env
Upload-HtmlReportArtifact.js->>Twirp API: CreateArtifact(mime_type=text/html)
Twirp API-->>Upload-HtmlReportArtifact.js: signed_upload_url
Upload-HtmlReportArtifact.js->>signed_upload_url: PUT HTML bytes
Upload-HtmlReportArtifact.js->>Twirp API: FinalizeArtifact(hash, size)
Twirp API-->>Upload-HtmlReportArtifact.js: artifact_id
Upload-HtmlReportArtifact.js->>actions/github-script: setOutput("artifact-url", url)
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as low quality.
This comment was marked as low quality.
There was a problem hiding this comment.
Pull request overview
This PR extends the Maester composite GitHub Action to (1) support uploading the HTML report as a standalone artifact rendered inline by GitHub, and (2) make the GitHub step summary output configurable by detail level.
Changes:
- Add a Node script that uploads the self-contained HTML report via GitHub’s Twirp Artifact API (non-zip,
text/html). - Change
step_summaryfrom a boolean to a multi-mode option (Full/Summary/Table/false) and generate compact summary formats. - Update action inputs and README to document the new options and HTML artifact upload input.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
script/Upload-HtmlReportArtifact.js |
New helper to upload HTML as a standalone artifact using the runtime token/results API. |
script/Run-MaesterAction.ps1 |
Adds step summary “modes” and compact summary generation paths. |
action.yml |
Updates step_summary input contract, adds artifact_upload_html, and adds steps to upload/link the HTML report. |
README.md |
Documents new step_summary behavior and the new HTML artifact upload input. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
script/Upload-HtmlReportArtifact.js (2)
93-143: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail soft on unexpected upload exceptions.
The script warns and returns on non-OK responses, but network failures, JSON parse failures, and blob upload exceptions still throw and fail the workflow after Maester has completed. Wrap the optional upload sequence and convert unexpected errors to
core.warning.Proposed fix shape
- const createResp = await fetch(`${origin}/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact`, { + try { + const createResp = await fetch(`${origin}/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact`, { method: 'POST', headers: authHeaders, body: JSON.stringify({ workflow_run_backend_id: runBackendId, workflow_job_run_backend_id: jobBackendId, name: artifactName, version: 7, mime_type: 'text/html' }) - }); + }); + // existing create/upload/finalize handling... + } catch (err) { + core.warning(`HTML artifact upload failed: ${err.message}`); + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/Upload-HtmlReportArtifact.js` around lines 93 - 143, Wrap the optional artifact upload flow in Upload-HtmlReportArtifact.js so unexpected exceptions are converted to core.warning instead of failing the workflow after Maester completes. In the sequence that calls CreateArtifact, reads the file, PUTs to signedUploadUrl, and FinalizeArtifact, catch network/JSON/blob upload errors around the existing createResp/blobResp/finalizeResp handling and log the failure with context, then return early just like the current non-OK paths.
108-142: 🎯 Functional Correctness | 🟠 MajorUse the camelCase response fields here.
CreateArtifactResponseexposessignedUploadUrl, andFinalizeArtifactResponseexposesartifactId; the currentsigned_upload_url/artifact_iddestructuring leaves both undefined and breaks the upload flow. Add a missing-field warning before using either value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/Upload-HtmlReportArtifact.js` around lines 108 - 142, The upload flow is destructuring the wrong response fields, so `createResp.json()` and `finalizeResp.json()` are leaving `signedUploadUrl` and `artifactId` undefined. Update `Upload-HtmlReportArtifact.js` to use the camelCase properties from `CreateArtifactResponse` and `FinalizeArtifactResponse`, and add a warning check before using either value so missing fields are reported early. Keep the fix localized around the artifact creation/finalization logic and the existing `signedUploadUrl` / `artifactId` references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@action.yml`:
- Around line 199-200: The step-summary link gate is too loose and does not
match the normalization in Run-MaesterAction.ps1. Update the condition on the
“Add HTML report link to step summary” step so it only runs when the summary
mode is actually enabled using the same allowed values as the PowerShell logic
(true, Full, Summary, Table), and not for other inputs like Disabled. Use the
existing inputs.step_summary check in action.yml to align the GitHub Actions
gate with the mode handling in Run-MaesterAction.ps1.
- Around line 196-197: The `require()` call in the upload step is inlining
`github.action_path`, which can break on Windows because backslashes may be
treated as escape sequences before the module loads. Update the step that calls
`upload({ core })` to pass the action path through `env`, then build the script
path inside the `require()` logic using `path.join()` so the module resolves
safely across platforms.
---
Outside diff comments:
In `@script/Upload-HtmlReportArtifact.js`:
- Around line 93-143: Wrap the optional artifact upload flow in
Upload-HtmlReportArtifact.js so unexpected exceptions are converted to
core.warning instead of failing the workflow after Maester completes. In the
sequence that calls CreateArtifact, reads the file, PUTs to signedUploadUrl, and
FinalizeArtifact, catch network/JSON/blob upload errors around the existing
createResp/blobResp/finalizeResp handling and log the failure with context, then
return early just like the current non-OK paths.
- Around line 108-142: The upload flow is destructuring the wrong response
fields, so `createResp.json()` and `finalizeResp.json()` are leaving
`signedUploadUrl` and `artifactId` undefined. Update
`Upload-HtmlReportArtifact.js` to use the camelCase properties from
`CreateArtifactResponse` and `FinalizeArtifactResponse`, and add a warning check
before using either value so missing fields are reported early. Keep the fix
localized around the artifact creation/finalization logic and the existing
`signedUploadUrl` / `artifactId` references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f3a956ac-cf54-47f6-a7ad-03d485004495
📒 Files selected for processing (4)
README.mdaction.ymlscript/Run-MaesterAction.ps1script/Upload-HtmlReportArtifact.js
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- script/Run-MaesterAction.ps1
…false positives - Pass github.action_path via env + path.join so backslashes on Windows runners are not mangled as JS escape sequences in the inline script - Gate the HTML-report summary link on the same enabled modes that Run-MaesterAction.ps1 normalizes (true/Full/Summary/Table) - Add justified eslint/semgrep suppressions for path-traversal and XSS findings that are false positives (path is confined to GITHUB_WORKSPACE by resolveReportPath; MAESTER_HTML_PATH is a file path, not HTML) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Review follow-up: findings, decisions, and fixes (pushed in 33136ea)A full pass was done over all changes, commit history, and every review conversation (including resolved ones). Summary of each open question, the evidence, and the confidence level of the decision: 1. Windows
|
| Rule | Lines | Assessment |
|---|---|---|
Semgrep + ESLint non-literal-fs-filename (x4) |
59, 111 | filePath is not user input (set to a literal in action.yml) and is confined to GITHUB_WORKSPACE by resolveReportPath() before use - the canonical CWE-22 mitigation |
Semgrep path-join-resolve-traversal |
25 | Fires on the sanitizer itself - the resolve + relative containment check is the mitigation these rules ask for |
ESLint xss/no-mixed-html |
54 | Naming heuristic only: a variable containing "Html" passed to a function. It is a file path; no HTML is ever rendered |
Codacy''s own metadata marks each of these patterns with an 80% false-positive threshold. Inline eslint-disable-next-line / nosemgrep suppressions with written justifications were added so the rationale lives in the code and survives re-analysis. If Codacy still gates on them, a repo admin should mark the six issues as ignored in the Codacy dashboard.
4. Undocumented Twirp Artifact API — Kept, no change · Confidence: Medium
Evidence: No publicly documented API provides inline-rendered single-file artifacts: the official @actions/artifact / actions/upload-artifact always ZIP-wrap (losing inline rendering), and GitHub Pages would publish security test results. The script already degrades gracefully (warnings only; the link step is skipped when no URL is produced).
Decision: Accept the fragility consciously - the file header already documents that this calls the internal Twirp API. Maintainers should be aware it may break without notice if GitHub changes internals.
Remaining before merge
- Codacy gate re-check on the new commit (suppressions may need dashboard dismissal as well)
- Re-review/approval from @merill (review is currently
CHANGES_REQUESTED)
All other checks pass (CodeQL, script analysis, live Maester runs on latest + preview).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
script/Upload-HtmlReportArtifact.js (2)
103-118: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the artifact upload best-effort on thrown failures.
The non-OK paths degrade gracefully, but
fetch(),response.json(), a missingsigned_upload_url, or a missingartifact_idcan still throw or emit anundefinedartifact link. Catch those paths and warn/return.Proposed guard pattern
+async function fetchOrWarn(core, label, url, options) { + try { + return await fetch(url, options); + } catch (error) { + core.warning(`${label} request failed: ${error.message}`); + return null; + } +} + +async function jsonOrWarn(core, label, response) { + try { + return await response.json(); + } catch (error) { + core.warning(`${label} returned invalid JSON: ${error.message}`); + return null; + } +} + - const createResp = await fetch(`${origin}/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact`, { + const createResp = await fetchOrWarn(core, 'CreateArtifact', `${origin}/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact`, { method: 'POST', headers: authHeaders, body: JSON.stringify({ workflow_run_backend_id: runBackendId, workflow_job_run_backend_id: jobBackendId, @@ mime_type: 'text/html' }) }); + if (!createResp) { + return; + } if (!createResp.ok) { core.warning(`CreateArtifact failed (${createResp.status}): ${await createResp.text()}`); return; } - const { signed_upload_url: signedUploadUrl } = await createResp.json(); + const createBody = await jsonOrWarn(core, 'CreateArtifact', createResp); + const signedUploadUrl = createBody?.signed_upload_url; + if (!signedUploadUrl) { + core.warning('CreateArtifact did not return a signed upload URL.'); + return; + } @@ - const blobResp = await fetch(signedUploadUrl, { + const blobResp = await fetchOrWarn(core, 'Blob upload', signedUploadUrl, { method: 'PUT', headers: { 'Content-Type': 'text/html', 'x-ms-blob-type': 'BlockBlob' }, body: fileBytes }); + if (!blobResp) { + return; + } @@ - const finalizeResp = await fetch(`${origin}/twirp/github.actions.results.api.v1.ArtifactService/FinalizeArtifact`, { + const finalizeResp = await fetchOrWarn(core, 'FinalizeArtifact', `${origin}/twirp/github.actions.results.api.v1.ArtifactService/FinalizeArtifact`, { method: 'POST', headers: authHeaders, body: JSON.stringify({ workflow_run_backend_id: runBackendId, workflow_job_run_backend_id: jobBackendId, @@ hash: `sha256:${sha256}` }) }); + if (!finalizeResp) { + return; + } if (!finalizeResp.ok) { core.warning(`FinalizeArtifact failed (${finalizeResp.status}): ${await finalizeResp.text()}`); return; } - const { artifact_id: artifactId } = await finalizeResp.json(); + const finalizeBody = await jsonOrWarn(core, 'FinalizeArtifact', finalizeResp); + const artifactId = finalizeBody?.artifact_id; + if (!artifactId) { + core.warning('FinalizeArtifact did not return an artifact ID.'); + return; + }Also applies to: 125-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/Upload-HtmlReportArtifact.js` around lines 103 - 118, The Upload-HtmlReportArtifact flow is still failing hard on thrown paths even though non-OK responses are handled gracefully. In the artifact creation/upload logic around the fetch/response handling in Upload-HtmlReportArtifact, wrap the response parsing and subsequent signed URL/artifact ID usage in best-effort guards so fetch(), createResp.json(), missing signed_upload_url, and missing artifact_id all emit a core.warning and return early instead of throwing or producing an undefined link. Reference the existing createResp, signedUploadUrl, and artifactId handling and keep the upload path resilient in the same style as the current non-OK checks.
25-35: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winClose the symlink escape in workspace confinement.
The
path.resolve/relativecheck blocks lexical traversal, butreadFileSyncwill still follow a symlink underGITHUB_WORKSPACEto a target outside the workspace. Resolve the real file path, or reject symlinks, before reading.Proposed hardening
+function isInsideBase(baseDir, targetPath) { + const relative = path.relative(baseDir, targetPath); + return relative === '' || (!relative.startsWith('..') && !path.isAbsolute(relative)); +} + function resolveReportPath(rawPath) { - const baseDir = path.resolve(process.env.GITHUB_WORKSPACE || process.cwd()); + const baseDir = fs.realpathSync.native(path.resolve(process.env.GITHUB_WORKSPACE || process.cwd())); // This resolve+relative check IS the CWE-22 mitigation: anything escaping // baseDir is rejected below, so the taint warning is a false positive. // nosemgrep: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal const resolved = path.resolve(baseDir, rawPath); - const relative = path.relative(baseDir, resolved); - if (relative.startsWith('..') || path.isAbsolute(relative)) { + if (!isInsideBase(baseDir, resolved)) { return null; } - return resolved; + return { baseDir, filePath: resolved }; } module.exports = async ({ core }) => { - const filePath = resolveReportPath(process.env.MAESTER_HTML_PATH || 'test-results/test-results.html'); - if (!filePath) { + const reportPath = resolveReportPath(process.env.MAESTER_HTML_PATH || 'test-results/test-results.html'); + if (!reportPath) { core.warning('HTML report path is outside the workspace; refusing to upload.'); return; } + const { baseDir, filePath } = reportPath; + + let realFilePath; + try { + realFilePath = fs.realpathSync.native(filePath); + } catch { + core.warning(`HTML report not found: ${filePath}`); + return; + } + if (!isInsideBase(baseDir, realFilePath)) { + core.warning('HTML report path resolves outside the workspace; refusing to upload.'); + return; + } - if (!fs.existsSync(filePath)) { - core.warning(`HTML report not found: ${filePath}`); - return; - } - const fileBytes = fs.readFileSync(filePath); + const fileBytes = fs.readFileSync(realFilePath);Also applies to: 62-69, 121-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/Upload-HtmlReportArtifact.js` around lines 25 - 35, The workspace confinement check in resolveReportPath only prevents lexical path traversal and still allows symlink escapes when the file is later read. Harden the file lookup by resolving the real filesystem path or rejecting symlinks before any readFileSync call, and apply the same fix to the other report-path usages noted in the diff so all access stays inside GITHUB_WORKSPACE.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@script/Upload-HtmlReportArtifact.js`:
- Around line 103-118: The Upload-HtmlReportArtifact flow is still failing hard
on thrown paths even though non-OK responses are handled gracefully. In the
artifact creation/upload logic around the fetch/response handling in
Upload-HtmlReportArtifact, wrap the response parsing and subsequent signed
URL/artifact ID usage in best-effort guards so fetch(), createResp.json(),
missing signed_upload_url, and missing artifact_id all emit a core.warning and
return early instead of throwing or producing an undefined link. Reference the
existing createResp, signedUploadUrl, and artifactId handling and keep the
upload path resilient in the same style as the current non-OK checks.
- Around line 25-35: The workspace confinement check in resolveReportPath only
prevents lexical path traversal and still allows symlink escapes when the file
is later read. Harden the file lookup by resolving the real filesystem path or
rejecting symlinks before any readFileSync call, and apply the same fix to the
other report-path usages noted in the diff so all access stays inside
GITHUB_WORKSPACE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 04d1b928-e39f-4f1a-8645-767e3ad56c68
📒 Files selected for processing (2)
action.ymlscript/Upload-HtmlReportArtifact.js
🚧 Files skipped from review as they are similar to previous changes (1)
- action.yml
The output URL is the artifact URL on the workflow run (rendered inline because the artifact is uploaded with mime_type=text/html), not a direct file URL. Align the JS header, input description, README, and step summary link text with what the link actually does. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…rd, quote summary path - Normalize step_summary input to a mode once (before the markdown writer import gate) so empty/unknown values consistently disable the summary, matching the later write logic - Replace existsSync with statSync + isFile so a directory path warns instead of readFileSync throwing EISDIR and failing the step - Quote \ redirects in bash Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Description
Your checklist for this pull request
🚨Please review the guidelines for contributing to this repository.
Review
We will try to review your pull request as soon as possible.
💖 Thank you!
Summary by CodeRabbit
artifact_upload_htmloption to upload a self-contained HTML report as a separate artifact and link it from the step summary.Full,Summary,Table, orfalse(legacytruemaps toFull).Tableand improved truncation for consistent output sizing.