[rendering-scripts] Render message/reason fields in safe-outputs step-summary preview - #53087
Conversation
noop, missing_tool, and missing_data safe outputs store their primary content in message/reason fields rather than title/body, so the preview silently rendered only the type name with no content for these types. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
automated-fix
javascript
🎉 Great work on this rendering fix!This PR looks solid — it tackles a real rendering gap where What we are seeing:
The changes are clean and ready for merge review. Well done tracking this down from the daily verification runs!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Diff mirrors existing title/body duplication pattern in the same function; message/reason handling is symmetric, minimal, and adds no new abstractions, deps, or dead flexibility.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes are correct and well-tested. Both plain-text and markdown rendering of message/reason fields are consistent with the existing body handling pattern (newline-flattening for plain-text, no flattening for markdown). Tests cover all three new code paths (noop message plain-text, noop message markdown, missing_tool reason). LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 11.6 AIC · ⌖ 7.88 AIC · ⊞ 5.6K
|
✅ PR Code Quality Reviewer completed the code quality review.
|
There was a problem hiding this comment.
Pull request overview
Extends safe-output previews so diagnostic content from noop, missing_tool, and missing_data entries is visible.
Changes:
- Renders and truncates
messageandreasonfields in plain-text and Markdown previews. - Adds regression coverage for both output formats.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/log_parser_shared.cjs |
Adds message/reason preview rendering. |
actions/setup/js/log_parser_shared.test.cjs |
Tests the new rendering behavior. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — one minor test gap found; otherwise approving.
📋 Key Themes & Highlights
Key Themes
- Test name/coverage mismatch: The new
missing_tool/missing_datatest only exercisesmissing_tool, leavingmissing_data'sreasonpath implicitly trusted but untested. - Truncation limits are consistent: plain-text
message/reasonuse 80 chars (matchingbody); markdown uses 200 chars (matchingbody). ✅
Positive Highlights
- ✅ Excellent root-cause diagnosis: PR traces the gap from real production run → log parser → field schema → fix
- ✅ Additive, backward-compatible change — existing entries unaffected (confirmed by the unchanged existing tests)
- ✅ Both rendering branches (plain-text and markdown) are handled symmetrically
- ✅ Regression tests added for
message(noop) andreason(missing_tool) in both modes
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 30 AIC · ⌖ 6.79 AIC · ⊞ 7.7K
Comment /matt to run again
| expect(plain).toContain("Reason: Docker is not available"); | ||
| expect(markdown).toContain("**Reason:** Docker is not available"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] Test name promises missing_tool/missing_data coverage but only exercises missing_tool — missing_data has a different schema (data_type, context, alternatives) and its reason rendering is untested.
💡 Suggested addition
it('should surface the reason field for missing_data entries', async () => {
const { formatSafeOutputsPreview } = await import('./log_parser_shared.cjs');
const safeOutputs = JSON.stringify({ type: 'missing_data', data_type: 'pr_number', reason: 'PR number not found in context' });
const plain = formatSafeOutputsPreview(safeOutputs, { isPlainText: true });
const markdown = formatSafeOutputsPreview(safeOutputs, { isPlainText: false });
expect(plain).toContain('Reason: PR number not found in context');
expect(markdown).toContain('**Reason:** PR number not found in context');
});@copilot please address this.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
Non-blocking change; I didn't find a correctness or maintainability issue in the diff.
Notes
The implementation is additive, preserves the existing title/body/data rendering paths, and the new tests cover both plain-text and markdown output for the new fields.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 3.27 AIC · ⌖ 6.13 AIC · ⊞ 6.9K
Comment /review to run again
|
🎉 This pull request is included in a new release. Release: |
Summary
The
formatSafeOutputsPreviewfunction inlog_parser_shared.cjsdid not render themessageorreasonfields carried bynoop,missing_tool, andmissing_datasafe-output entries, leaving their primary content invisible in step-summary previews. This change adds rendering for both fields in plain-text and markdown preview modes, with matching test coverage.Change Classification
Key Changes
actions/setup/js/log_parser_shared.cjsMessage:/Reason:line rendering (plain text) and**Message:**/**Reason:**blocks (markdown) for entries withmessageorreasonfieldsactions/setup/js/log_parser_shared.test.cjsnoopmessage rendering (plain text and markdown) andmissing_tool/missing_datareason renderingImpact Assessment
noop,missing_tool, andmissing_datasafe-output entries now surface their diagnosticmessage/reasontext (truncated to 80 chars in plain text, 200 chars in markdown), improving visibility for workflow debugging.title/bodyrendering paths; purely additive to the preview formatter.Commits