Skip to content

fix: inject threat-detection CAUTION alert at top of markdown content#29264

Open
Copilot wants to merge 3 commits intomainfrom
copilot/update-caution-alert-position
Open

fix: inject threat-detection CAUTION alert at top of markdown content#29264
Copilot wants to merge 3 commits intomainfrom
copilot/update-caution-alert-position

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 30, 2026

The [!CAUTION] security alert for threat detection was appended as part of footer generation, burying it at the bottom of content where it could easily be missed. It should appear at the very top so reviewers see it immediately.

Strategy

Every content-building path now follows the pattern already established in create_pull_request.cjs and handle_agent_failure.cjs:

  1. Call getDetectionCautionAlert() / getExpiredEntityCautionAlert()
  2. Prepend to the body before any user content
  3. Pass { skipDetectionCaution: true } to generateFooterWithMessages() to prevent duplication
// Before
processedBody += "\n\n" + generateFooterWithMessages(...); // CAUTION buried in footer

// After
const detectionCaution = getDetectionCautionAlert(workflowName, runUrl);
if (detectionCaution) {
  processedBody = detectionCaution + "\n\n" + processedBody;
}
processedBody += "\n\n" + generateFooterWithMessages(..., { skipDetectionCaution: true });

Changes by file

  • generate_footer.cjs — Extracted CAUTION logic out of generateExpiredEntityFooter into a new exported getExpiredEntityCautionAlert(). The footer itself no longer includes CAUTION; callers are responsible for prepending it. Note is kept inline (not imported from messages_footer.cjs) due to bundler constraints that would break tests.

  • messages_footer.cjs — Removed the CAUTION prepend from getFooterWorkflowRecompileMessage and getFootorWorkflowRecompileCommentMessage; callers now own placement.

  • close_expired_{issues,pull_requests,discussions}.cjs — Use getExpiredEntityCautionAlert and prepend to the closing message before the expiration text.

  • update_activation_comment.cjs — Reordered template: cautionSection + body + footer (was body + cautionSection + footer).

  • create_{issue,discussion}.cjsunshift CAUTION lines to the front of bodyLines before joining.

  • add_comment.cjs, comment_memory.cjs, close_entity_helpers.cjs, mark_pull_request_as_ready_for_review.cjs, check_workflow_recompile_needed.cjs, pr_review_buffer.cjs, reply_to_pr_review_comment.cjs, update_pr_description_helpers.cjs — Each prepends CAUTION to its content and passes skipDetectionCaution: true to its footer call.

Tests

  • add_comment.test.cjs — Updated position assertion: CAUTION index must be < body-content index.
  • generate_footer.test.cjs — Updated to assert CAUTION is not in generateExpiredEntityFooter; added tests for the new getExpiredEntityCautionAlert function.

Copilot AI and others added 2 commits April 30, 2026 05:43
…oints

The threat detection CAUTION alert was previously injected near the footer
instead of at the top of the markdown content. This change moves it to the
top across all places where it's injected:

- add_comment.cjs: prepend CAUTION before body, pass skipDetectionCaution:true to footer
- update_activation_comment.cjs: move cautionSection before body in link/commit messages
- generate_footer.cjs: extract CAUTION from generateExpiredEntityFooter into
  getExpiredEntityCautionAlert (new exported function), callers responsible for placement
- close_expired_issues/pull_requests/discussions.cjs: use getExpiredEntityCautionAlert
  and prepend to closing message
- create_issue.cjs / create_discussion.cjs: unshift CAUTION to top of bodyLines
- comment_memory.cjs: prepend CAUTION before managed comment header
- close_entity_helpers.cjs: prepend CAUTION in buildCommentBody
- mark_pull_request_as_ready_for_review.cjs: prepend CAUTION before sanitized reason
- check_workflow_recompile_needed.cjs: prepend CAUTION to comment/issue body
- pr_review_buffer.cjs: prepend CAUTION before review body
- reply_to_pr_review_comment.cjs: prepend CAUTION before reply body
- update_pr_description_helpers.cjs: prepend CAUTION in updateBody for all operations
- messages_footer.cjs: remove CAUTION from getFooterWorkflowRecompile* functions

Tests updated: add_comment.test.cjs (CAUTION position assertion),
generate_footer.test.cjs (CAUTION no longer in generateExpiredEntityFooter,
new tests for getExpiredEntityCautionAlert)

Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a07d9fd2-8563-4c82-9b25-b766b139b7f6

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@pelikhan pelikhan marked this pull request as ready for review April 30, 2026 05:50
Copilot AI review requested due to automatic review settings April 30, 2026 05:50
@github-actions github-actions Bot mentioned this pull request Apr 30, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 100/100

Excellent test quality

Metric Value
New/modified tests analyzed 5
✅ Design tests (behavioral contracts) 5 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 5 (100%)
Duplicate test clusters 0
Test inflation detected No
🚨 Coding-guideline violations None

Test Classification Details

View All Test Classifications (5 tests)
Test File Classification Issues Detected
Modified: CAUTION ordering assertion add_comment.test.cjs:2158 ✅ Design None — verifies positional ordering invariant
Modified: should NOT include caution alert... generate_footer.test.cjs:440 ✅ Design None — correctly inverts assertion for new behavior
should return caution alert when detection conclusion is warning generate_footer.test.cjs:471 ✅ Design None — verifies full content of returned alert
should return empty string when detection conclusion is not warning generate_footer.test.cjs:480 ✅ Design None — edge case branch coverage
should return empty string when detection conclusion is not set generate_footer.test.cjs:487 ✅ Design None — edge case: unset env var

Analysis Summary

All 5 new/modified tests enforce clear behavioral contracts:

  1. add_comment.test.cjs — The modified test upgrades from a weaker toContain assertion to an explicit positional ordering check (indexOf comparison), ensuring the CAUTION alert is injected before the comment body content. This is the core behavioral invariant of the PR and is well-covered.

  2. generate_footer.test.cjs — Three changes:

    • The existing test is correctly updated to assert the footer no longer contains [!CAUTION] (reflecting the responsibility shift to callers).
    • Three new tests for getExpiredEntityCautionAlert cover: the happy path (warning state), the alternate branch (non-warning state), and the unset-env edge case. Together they fully specify the new function's contract.

No mocking of internal business logic was detected. vi.resetModules() + dynamic import() is used for module isolation (acceptable pattern). No Go test files were changed.


Language Support

Tests analyzed:

  • 🐹 Go (*_test.go): 0 tests
  • 🟨 JavaScript (*.test.cjs): 5 tests (vitest)

Verdict

Check passed. 0% of new tests are implementation tests (threshold: 30%). All new tests verify observable behavioral contracts and include edge case coverage.


📖 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: §25149693646

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

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: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 5 new/modified tests verify observable behavioral contracts with edge case coverage.

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

Moves the threat-detection [!CAUTION] security alert out of footer generation and ensures it is surfaced at the beginning of user-visible Markdown content, avoiding duplication by passing skipDetectionCaution: true to generateFooterWithMessages().

Changes:

  • Prepend threat-detection CAUTION to generated bodies/comments/reviews and skip CAUTION in the footer to prevent duplicates.
  • Extract expired-entity CAUTION generation into getExpiredEntityCautionAlert() and remove CAUTION from generateExpiredEntityFooter().
  • Update/extend tests to assert CAUTION placement and new helper behavior.
Show a summary per file
File Description
actions/setup/js/update_pr_description_helpers.cjs Adds CAUTION injection and skips footer CAUTION when building updated bodies.
actions/setup/js/update_activation_comment.cjs Reorders activation comment template to place CAUTION before main body.
actions/setup/js/reply_to_pr_review_comment.cjs Prepends CAUTION when footer is included and skips footer CAUTION.
actions/setup/js/pr_review_buffer.cjs Prepends CAUTION before adding footer in buffered PR reviews; skips footer CAUTION.
actions/setup/js/messages_footer.cjs Removes CAUTION prepend from workflow-recompile footer helpers (callers now own placement).
actions/setup/js/mark_pull_request_as_ready_for_review.cjs Prepends CAUTION and skips footer CAUTION.
actions/setup/js/generate_footer.cjs Adds getExpiredEntityCautionAlert() and removes CAUTION from expired-entity footer generation.
actions/setup/js/generate_footer.test.cjs Updates assertions to ensure expired-entity footer no longer includes CAUTION; adds tests for new helper.
actions/setup/js/create_issue.cjs Prepends CAUTION to issue body lines; skips footer CAUTION.
actions/setup/js/create_discussion.cjs Prepends CAUTION to discussion body lines; skips footer CAUTION.
actions/setup/js/comment_memory.cjs Prepends CAUTION to managed memory comment body; skips footer CAUTION.
actions/setup/js/close_expired_pull_requests.cjs Prepends expired-entity CAUTION before expiration text; footer no longer includes CAUTION.
actions/setup/js/close_expired_issues.cjs Prepends expired-entity CAUTION before expiration text; footer no longer includes CAUTION.
actions/setup/js/close_expired_discussions.cjs Prepends expired-entity CAUTION before expiration text; footer no longer includes CAUTION.
actions/setup/js/close_entity_helpers.cjs Prepends CAUTION to close-comment bodies and skips footer CAUTION.
actions/setup/js/check_workflow_recompile_needed.cjs Prepends CAUTION to recompile issue/comment bodies (since recompile footer helpers no longer include it).
actions/setup/js/add_comment.cjs Prepends CAUTION to comment body and skips footer CAUTION.
actions/setup/js/add_comment.test.cjs Updates test to assert CAUTION appears before the body content.

Copilot's findings

Tip

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

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

Comment on lines +99 to +102
// Inject CAUTION at top of new content if threat detection warning was raised
const detectionCaution = getDetectionCautionAlert(workflowName, runUrl);
const contentWithCaution = detectionCaution ? detectionCaution + "\n\n" + sanitizedNewContent : sanitizedNewContent;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's ok to only be in tbe island

Comment on lines +159 to +167
// Append footer with workflow information when enabled
let finalBody = sanitizeContent(body);
if (includeFooter) {
const footer = generateFooterWithMessages(workflowName, runUrl, workflowSource, workflowSourceURL, undefined, triggeringPRNumber, undefined);
// Inject CAUTION at top of body if threat detection warning was raised
const detectionCaution = getDetectionCautionAlert(workflowName, runUrl);
if (detectionCaution) {
finalBody = detectionCaution + "\n\n" + finalBody;
}
const footer = generateFooterWithMessages(workflowName, runUrl, workflowSource, workflowSourceURL, undefined, triggeringPRNumber, undefined, undefined, { skipDetectionCaution: true });
Comment on lines 246 to +260
// Determine if we should add footer based on footer mode
let shouldAddFooter = footerMode === "always";
if (footerMode === "if-body") {
// Only add footer if body is non-empty (has meaningful content)
shouldAddFooter = body.trim().length > 0;
core.info(`Footer mode "if-body": body is ${body.trim().length > 0 ? "non-empty" : "empty"}, ${shouldAddFooter ? "adding" : "skipping"} footer`);
}

// Add footer to review body if we should and we have footer context
if (shouldAddFooter && footerContext) {
// Inject CAUTION at top of body if threat detection warning was raised
const detectionCaution = getDetectionCautionAlert(footerContext.workflowName, footerContext.runUrl);
if (detectionCaution) {
body = detectionCaution + "\n\n" + body;
}
@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot review all comments

… pr_review_buffer

- reply_to_pr_review_comment.cjs: move CAUTION injection outside includeFooter
  block so it appears even when footer is disabled
- pr_review_buffer.cjs: move CAUTION injection before the shouldAddFooter guard;
  when CAUTION is present, force shouldAddFooter=true so footer metadata is
  also included

Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d7cb63e7-66e8-40b7-af27-34bb96a8fc42

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 30, 2026

@copilot review all comments

Addressed both open review comments in 7e6ffa9:

  • reply_to_pr_review_comment.cjs: moved CAUTION injection outside the includeFooter block so it's applied unconditionally regardless of footer settings.
  • pr_review_buffer.cjs: moved CAUTION injection before the shouldAddFooter && footerContext guard. When a detection warning is present, shouldAddFooter is also forced to true so the footer metadata is included alongside the alert.

update_pr_description_helpers.cjs left as-is per the "ok to only be in the island" feedback.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • invalid.example.invalid
    • Triggering command: /usr/lib/git-core/git-remote-https /usr/lib/git-core/git-remote-https origin https://invalid.example.invalid/nonexistent-repo.git git conf�� --local lure rgo/bin/git user.name h ules/.bin/git git add . git 1/x64/bin/git -M main k/node_modules/.agent-change.txt git (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from pelikhan April 30, 2026 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants