Add deterministic body footer templates to safe outputs - #58841
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
This footer is different from the other template footer defined in messages |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review for #58841 and submitted review output via safeoutputs.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff 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
- "github.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! 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
- "ab.chatgpt.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
🟡 Changes recommended
Fallback footer ordering and body-length validation remain unresolved, with fallback coverage also incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds configurable deterministic body footers to issue and pull-request safe outputs.
Changes:
- Adds
body-footerconfiguration with workflow placeholders. - Supports footer rendering, metadata ordering, and fallback handling.
- Adds tests, documentation, typings, schema updates, and release metadata.
File summaries
| File | Description |
|---|---|
pkg/workflow/safe_outputs_handler_registry_pull_requests.go |
Serializes PR footer configuration. |
pkg/workflow/safe_outputs_handler_registry_issues.go |
Serializes issue footer configuration. |
pkg/workflow/safe_outputs_footer_test.go |
Tests configuration propagation. |
pkg/workflow/safe_outputs_config_types.go |
Defines the footer field. |
pkg/parser/schemas/main_workflow_schema.json |
Adds schema entries. |
pkg/parser/schema_test.go |
Tests schema acceptance. |
docs/src/content/docs/reference/footers.md |
Documents deterministic footers. |
actions/setup/js/types/safe-outputs-config.d.ts |
Adds TypeScript declarations. |
actions/setup/js/messages.test.cjs |
Tests template rendering. |
actions/setup/js/messages.cjs |
Exports the footer renderer. |
actions/setup/js/messages_footer.cjs |
Implements footer rendering. |
actions/setup/js/create_pull_request.test.cjs |
Tests standard PR footer behavior. |
actions/setup/js/create_pull_request.cjs |
Applies PR and fallback footers. |
actions/setup/js/create_issue.test.cjs |
Tests issue footer behavior. |
actions/setup/js/create_issue.cjs |
Applies issue footers. |
.changeset/patch-add-safe-output-body-footer.md |
Records the patch release. |
Review details
Suppressed comments (2)
actions/setup/js/create_pull_request.cjs:1712
- The new fallback-specific behavior is not exercised by the added PR test: that test only reaches the successful
pulls.createpath. Please extend the existing protected-filesfallback-to-issuetests (including the push-failed variant) with abody_footercontaining a closing keyword and assert that it remains at the end of visible content and that the keyword is escaped; these split-body paths can otherwise regress independently of normal PR creation.
const issueSafeBody = neutralizeClosingKeywordsForIssueBody(body);
// Footer section (footer + workflow-id marker) used when ordering protected-files notices
const footerContent = footerParts.join("\n\n");
const issueSafeFooterContent = neutralizeClosingKeywordsForIssueBody(footerContent);
actions/setup/js/create_pull_request.test.cjs:1474
- The added test covers only successful PR creation, so the new protected-file fallback propagation and closing-keyword neutralization are untested even though those paths have substantial existing test coverage. Add fallback-to-issue tests for both push-success and push-failure variants, asserting that the configured footer is present at the end of visible content and that closing keywords in it are escaped.
it("should append the configured body footer when the generated footer is disabled", async () => {
- Files reviewed: 15/16 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| const bodyFooter = getBodyFooterMessage(config.body_footer, { workflowName, runUrl }); | ||
| if (bodyFooter) { | ||
| const renderedBodyFooter = bodyFooter.trimEnd(); | ||
| bodyLines.push(``, renderedBodyFooter); |
| GitHubApp *GitHubAppConfig `yaml:"github-app,omitempty"` // GitHub App credentials for minting a per-handler installation access token | ||
| Staged *TemplatableBool `yaml:"staged,omitempty"` // Templatable preview-only mode for this specific output type | ||
| Footer *string `yaml:"footer,omitempty"` // Controls AI-generated footer behavior. Values vary by handler; false omits the visible footer but keeps XML markers. | ||
| BodyFooter string `yaml:"body-footer,omitempty"` // Deterministic template appended after agent-generated body content by supported handlers. |
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new body-footer plumbing is close, but the protected-files fallback path still has no targeted coverage.
Blocking theme
This feature now depends on a second body-construction path for fallback issues, including keyword neutralization, and the diff does not prove that path preserves the configured footer. That gap is enough to let a regression through unnoticed.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 35.3 AIC · ⌖ 7.39 AIC · ⊞ 21.8K
Comment /review to run again
| const issueSafeBody = neutralizeClosingKeywordsForIssueBody(body); | ||
| // Footer section (footer + workflow-id marker) used when ordering protected-files notices | ||
| const footerContent = footerParts.join("\n\n"); | ||
| const issueSafeFooterContent = neutralizeClosingKeywordsForIssueBody(footerContent); |
There was a problem hiding this comment.
This adds a second footer code path for protected-file fallback issues, but the tests never exercise it, so a regression here will ship silently.
💡 Add a focused fallback-path test
create_pull_request.cjs now threads body_footer through issueSafeFooterContent and the manifest-protection fallback body builder, which is exactly the branch most likely to drift because it has its own body assembly and closing-keyword neutralization. Please add a test that forces the protected-files fallback issue path and asserts the configured body_footer is present after sanitization, otherwise this feature is only covered for the happy-path PR body.
There was a problem hiding this comment.
Simpler path: drop the extra body-footer helper and its public re-export so the new footer logic stays inline at the two call sites. net: -8 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #58841 · codex · mai10 · 5.23 AIC · ⌖ 10.7 AIC · ⊞ 13.5K
Comment /ponytail to run again
| * @param {{workflowName: string, runUrl: string}} ctx - Template context | ||
| * @returns {string} Rendered body footer, or an empty string when not configured | ||
| */ | ||
| function getBodyFooterMessage(template, ctx) { |
There was a problem hiding this comment.
actions/setup/js/messages_footer.cjs:302: yagni: new getBodyFooterMessage helper for one renderTemplate call. Inline it at the two call sites and drop the wrapper.
There was a problem hiding this comment.
Reviewed with the Impeccable framework, falling back to a standard correctness/security review since this PR is a backend (Go/Node) feature change with no UI surface.
Scope: Adds body-footer templating to create-issue and create-pull-request safe outputs, with ordering guarantees (footer always last, before hidden XML markers) and closing-keyword neutralization for fallback issue bodies.
Findings: No blocking issues found.
Review notes
body-footeris a workflow-author-controlled config value (not agent/untrusted content), so template interpolation viarenderTemplate's{key}substitution poses no injection risk.- Ordering is correctly enforced in both
create_issue.cjsandcreate_pull_request.cjs: body footer appended after the standard footer and before XML markers, verified by new tests (create_issue.test.cjs,create_pull_request.test.cjs). create_pull_request.cjscorrectly threadsissueSafeFooterContent(closing-keyword-neutralized) into the manifest-protection fallback issue body paths, consistent with existingissueSafeMainBodyContenthandling.- Schema, Go config plumbing (
BodyFooterfield, handler registries), and docs are consistent with the existingtitle-prefixpattern. - Nit (non-blocking):
getBodyFooterMessagereturns""(not trimmed) iftemplateis falsy, but iftemplateis a whitespace-only string it renders and is pushed (aftertrimEnd(), becomes empty), producing an extra blank line inbodyLines.push(``, bodyFooter.trimEnd()). Very low impact, not tied to changed core logic beyond this feature.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 48.5 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design to the new deterministic body-footer feature (getBodyFooterMessage, create_issue.cjs, create_pull_request.cjs). Overall the change is small, well-tested for the happy paths, and the docs/schema/changeset are all updated consistently.
📋 Key Themes & Highlights
Key Themes
- Missing regression test for neutralization order:
create_pull_request.cjsnow runsneutralizeClosingKeywordsForIssueBodyover the combinedfooterContent(which includes the newbody_footer) before it lands in protected-file fallback issues, but there's no test with abody_footercontaining a closing keyword to confirm it's actually neutralized in that path. - Sanitization asymmetry: the new footer content bypasses the existing
sanitizeContentstep used for the rest of the issue/PR body. Likely fine givenbody-footeris workflow-author-controlled, but worth a documenting comment or explicit note since it diverges from the established pattern for all other body content.
Positive Highlights
- ✅ Good placement/ordering tests confirming the footer lands after the generated footer and before hidden XML markers, for both issue and PR handlers.
- ✅
getBodyFooterMessagefollows the existingrenderTemplate/toSnakeCasepattern used elsewhere inmessages_footer.cjs, keeping the new code consistent with the module's established idioms. - ✅ Schema, type definitions, docs, and changeset are all updated together — nice completeness for a small feature addition.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 74.1 AIC · ⌖ 14.8 AIC · ⊞ 10.3K
Comment /matt to run again
| const issueSafeBody = neutralizeClosingKeywordsForIssueBody(body); | ||
| // Footer section (footer + workflow-id marker) used when ordering protected-files notices | ||
| const footerContent = footerParts.join("\n\n"); | ||
| const issueSafeFooterContent = neutralizeClosingKeywordsForIssueBody(footerContent); |
There was a problem hiding this comment.
[/tdd] New issueSafeFooterContent neutralizes closing keywords in the footer (which now includes the configured body-footer) before it reaches the protected-file fallback issue, but no test exercises this path with a body_footer containing a closing keyword (e.g. Fixes #1).
💡 Suggested test
Add a case to the manifest-protection fallback tests that configures body_footer: "Fixes #1" and asserts the resulting fallback issue body contains a neutralized keyword (e.g. `Fixes` #1 or similar) rather than an active closing reference. Without this, a regression that skips neutralization for the new footer content would silently auto-close unrelated issues.
@copilot please address this.
| bodyLines.push(``, footer); | ||
| } | ||
|
|
||
| const bodyFooter = getBodyFooterMessage(config.body_footer, { workflowName, runUrl }); |
There was a problem hiding this comment.
[/codebase-design] body_footer is appended to bodyLines directly, bypassing the sanitizeContent call applied to processedBody a few lines above (line 919). Since body-footer is workflow-author-controlled (not agent output), this is lower risk than the main body, but it's inconsistent with the codebase's existing security posture where all body content flows through sanitization before hitting bodyLines.
💡 Consideration
If body-footer templates ever interpolate values that could contain untrusted data (e.g. a future placeholder pulling from PR/issue titles), this unsanitized path would become an injection vector. Worth a one-line comment documenting the assumption that body-footer is always static/workflow-author-controlled and never agent-influenced.
@copilot please address this.
@copilot Please take the next forward-progress pass on this PR.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Implemented in ff8db86. Pull-request fallback issues now build from the main body and append the configured footer after visible recovery instructions, while preserving hidden metadata. Oversized combined issue/PR bodies are rejected before GitHub API calls; affected JavaScript tests and final validation passed. No blockers remain from this feedback. |
|
@copilot the new field is additive and a shared agentic workflow can define one and it will be appended together |
Correct—the field is additive. The configured |
|
🎉 This pull request is included in a new release. Release: |
Document the body-footer safe-output field added in #58841, which was missing from .github/aw instructions despite being documented on the public docs site. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Workflow authors can now define deterministic footer content for issues and pull requests, ensuring required text is appended regardless of agent-generated content.
Changes
Configuration
body-footertocreate-issueandcreate-pull-request.{workflow_name}and{run_url}placeholders.Rendering
Fallback handling