Skip to content

fix: safe_output_summary shows final posted body including footer for add_comment#29435

Merged
pelikhan merged 1 commit into
mainfrom
copilot/fix-safeoutputs-add-comment
May 1, 2026
Merged

fix: safe_output_summary shows final posted body including footer for add_comment#29435
pelikhan merged 1 commit into
mainfrom
copilot/fix-safeoutputs-add-comment

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 1, 2026

Summary

When an agent submits an add_comment via the safe outputs MCP tool, the "Body Preview" in the job step summary was showing only what the agent submitted (message.body), without the footer that add_comment.cjs appends during processing. This caused a mismatch between the "Body Preview" shown in the step summary and the actually posted comment (which includes the footer attribution).

Example mismatch visible at: https://github.com/github/gh-aw/actions/runs/25200713765/job/73891038812#step:37:1

Changes

  • add_comment.cjs: Include processedBody (the final body with footer appended) in the result returned by recordComment as body
  • safe_output_summary.cjs: Prefer result.body (final posted body including footer) over message.body (raw submitted body) for the Body Preview, falling back to message.body when result.body is absent
  • safe_output_summary.test.cjs: Add two tests verifying result.body preference and fallback behavior

… add_comment

When an agent submits an add_comment via the safe outputs MCP tool,
the body preview in the job step summary was showing only what the
agent submitted (message.body), without the footer that is appended
during processing. This caused a mismatch between the "Body Preview"
shown in the step summary and the actually posted comment (which
includes the footer attribution).

Fix:
- Return `processedBody` (the final body with footer) from
  add_comment.cjs in the result object as `body`
- Update safe_output_summary.cjs to prefer `result.body` (final
  posted body) over `message.body` (submitted body) for the Body
  Preview, falling back to message.body when result.body is absent
- Add two tests verifying the result.body preference and fallback

Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8069bd2d-dd4c-403e-96f3-9e2d9532f4a2

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI requested a review from pelikhan May 1, 2026 04:12
@pelikhan pelikhan marked this pull request as ready for review May 1, 2026 04:16
Copilot AI review requested due to automatic review settings May 1, 2026 04:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates safe-output step summaries so the “Body Preview” for add_comment reflects the final posted comment body (including the footer appended during processing), eliminating mismatches between the summary preview and the actual posted comment.

Changes:

  • Return the fully processed comment body (processedBody, including footer/markers) from add_comment as result.body.
  • Update safe_output_summary to prefer result.body over message.body for “Body Preview”, with a fallback to message.body.
  • Add tests covering the preference for result.body and the fallback behavior.
Show a summary per file
File Description
actions/setup/js/add_comment.cjs Includes the final processed comment body in the handler result so downstream consumers can display what was actually posted.
actions/setup/js/safe_output_summary.cjs Uses result.body (when present) for “Body Preview” to match the posted comment content.
actions/setup/js/safe_output_summary.test.cjs Adds coverage to ensure result.body is preferred and message.body is used as a fallback.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 0

@github-actions github-actions Bot mentioned this pull request May 1, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 1, 2026

🧪 Test Quality Sentinel Report

Test Quality Score: 75/100

⚠️ Acceptable — with suggestions

Metric Value
New/modified tests analyzed 2
✅ Design tests (behavioral contracts) 2 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 1 (50%)
Duplicate test clusters 0
Test inflation detected Yes (40 test lines vs 5 production lines, ~8:1 ratio)
🚨 Coding-guideline violations None

Test Classification Details

View Test Breakdown (2 tests)
Test File Classification Issues Detected
should use result.body (final posted body) over message.body for body preview safe_output_summary.test.cjs ✅ Design Happy-path only; tests core behavioral fix
should fall back to message.body when result.body is absent safe_output_summary.test.cjs ✅ Design Edge case: absent result.body fallback

Suggestions

⚠️ Test Inflation — safe_output_summary.test.cjs

The test file added 40 lines while production code added only 5 lines (~8:1 ratio, threshold 2:1). This is expected when adding comprehensive tests for a targeted fix, but worth noting. No action required if the test coverage is intentional.

💡 Consider an additional edge case

The fix covers when result.body is present or absent. A minor addition worth considering: test the case where result.body is an empty string "" to confirm whether the fallback logic handles falsy values correctly (if that is an expected production scenario).


Language Support

Tests analyzed:

  • 🟨 JavaScript (*.test.cjs): 2 tests (vitest)

Verdict

Check passed. 0% of new tests are implementation tests (threshold: 30%). Both new tests verify behavioral contracts — that the summary shows the final posted body (including footer) over the raw submitted body, and that the fallback behaves correctly when result.body is absent. The score deduction is solely from test inflation.


📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References: §25201970324

🧪 Test quality analysis by Test Quality Sentinel · ● 343.7K ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 75/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). Both tests verify behavioral contracts for the result.body fix.

@pelikhan
Copy link
Copy Markdown
Collaborator

pelikhan commented May 1, 2026

@copilot the generated comment only had "Test comment"

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 1, 2026

@copilot the generated comment only had "Test comment"

Yes — the agent submitted "Test comment", and the Body Preview in the step summary was faithfully showing that raw submitted body. But the actual posted comment had the footer appended by add_comment.cjs (the > 🔭 *Intelligence gathered by...* line), causing the mismatch.

The fix (commit 92f07c7) makes add_comment.cjs return processedBody (the final body including footer) in the result as body, and safe_output_summary.cjs now uses result.body for the Body Preview so it reflects what was actually posted.

@pelikhan pelikhan merged commit 7f3dd64 into main May 1, 2026
76 checks passed
@pelikhan pelikhan deleted the copilot/fix-safeoutputs-add-comment branch May 1, 2026 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants