Regularize conclusion no-op summaries - #51487
Conversation
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail review: reviewed conclusion_summary.cjs and its two call sites. Change extracts a small shared formatter used by 2 existing callers, straightforward validation logic (runUrl scheme check), no reinvented stdlib, no unneeded deps, no speculative abstraction. Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #51487 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Pull request overview
Regularizes conclusion-job no-op summaries to match safe-output summary formatting.
Changes:
- Adds a shared collapsible no-op summary formatter.
- Links valid workflow run URLs and redacts credential-shaped content.
- Updates both no-op handling paths with integration tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/conclusion_summary.cjs |
Implements shared summary formatting and redaction. |
actions/setup/js/conclusion_summary.test.cjs |
Tests formatting and URL handling. |
actions/setup/js/handle_noop_message.cjs |
Uses the shared formatter. |
actions/setup/js/handle_noop_message.test.cjs |
Verifies normal no-op summary output. |
actions/setup/js/notify_comment_error.cjs |
Uses the formatter for fallback summaries. |
actions/setup/js/notify_comment_error.test.cjs |
Tests the fallback summary path. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The refactoring cleanly extracts a shared buildNoopConclusionSummary helper and wires it into both handle_noop_message and notify_comment_error. The collapsible HTML structure, URL validation, and staged-mode flag are all handled correctly. Tests cover the key scenarios. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.7 AIC · ⌖ 7.13 AIC · ⊞ 5.5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — approving with one minor suggestion.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Clean extraction of
buildNoopConclusionSummaryinto a focused module — simple interface, consistent with existing patterns - ✅ URL validation (
/^https?:\/\//) guards against `(redacted) injection in run links; tested explicitly - ✅ Tests cover the main happy path, staged mode, and the security-relevant non-http URL case
- ✅
redactStepSummaryContentapplied at the formatter boundary so callers don't have to remember it - ✅
handle_noop_messageandnotify_comment_errornow share the same formatter — good DRY win
Minor Gap
- One edge case not yet covered:
buildNoopConclusionSummary([])(empty array). See inline comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 22.5 AIC · ⌖ 7.52 AIC · ⊞ 7.1K
Comment /matt to run again
| summary += staged ? "The following messages would be logged if staged mode was disabled:\n\n" : "The following messages were logged for transparency:\n\n"; | ||
| summary += target; | ||
|
|
||
| for (let index = 0; index < messages.length; index++) { |
There was a problem hiding this comment.
[/tdd] No test covers an empty messages array — buildNoopConclusionSummary([]) produces "0 no-op messages" with an empty body. Worth adding a quick test to document the intended behaviour and guard against future regressions.
💡 Suggested test
it('handles empty messages array gracefully', () => {
const summary = buildNoopConclusionSummary([]);
expect(summary).toContain('0 no-op messages');
expect(summary).not.toContain('No-Op - Success');
});@copilot please address this.
🧪 Test Quality SentinelPR: #51487 — "Regularize conclusion no-op summaries" SummaryThis PR introduces a new module ( Key Quality Signals:
Per-Test ClassificationClick to expand test analysis
Line Inflation Analysis:
Violations & Flags✅ No hard violations detected.
Soft Flag (monitoring):
Recommendation✅ ACCEPTABLE. Tests provide solid behavioral coverage with proper edge-case handling and resource management. Implementation test ratio is 0% (all are design tests), well below the 30% threshold. Proceed to merge.
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, re-check the current maintainer-facing state, run the Open review context (newest first):
Run: https://github.com/github/gh-aw/actions/runs/31287357931
|
|
🎉 This pull request is included in a new release. Release: |
Extends #51478's fix for #51477 to the summary emitted by the
conclusionjob for delegatednoopsafe outputs.Changes
Validation
GH_AW_PROMPTS_DIR="$PWD/../md" npx vitest run --no-file-parallelism conclusion_summary.test.cjs handle_noop_message.test.cjs notify_comment_error.test.cjs(59 tests passed)make fmt-cjs && make lint-cjs(passed)make agent-report-progresspassed formatting, build, schema freshness, lint, and Go checks; setup JavaScript tests remain blocked by 11 pre-existing TypeScript errors in unrelated files