safe-outputs: add per-output opt-in normalization for backticked issue-closing keywords#36701
safe-outputs: add per-output opt-in normalization for backticked issue-closing keywords#36701Copilot wants to merge 12 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds an opt-in safe-outputs.normalize-closing-keywords flag that propagates from workflow YAML → generated safe-outputs config.json → NDJSON ingestion, enabling deterministic normalization that strips backticks only around recognized GitHub issue-closing keyword references in body fields.
Changes:
- Introduces
NormalizeClosingKeywordsinSafeOutputsConfig, parses it from YAML, and merges it through imported workflow configs. - Emits
normalize_closing_keywordsinto generated safe-outputsconfig.jsonwhen enabled and plumbs the flag into NDJSON ingestion validation options. - Implements and tests targeted backtick-stripping normalization for closing-keyword references in
safe_output_type_validator.cjs.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_types.go | Adds NormalizeClosingKeywords to the workflow safe-outputs config type. |
| pkg/workflow/safe_outputs_config.go | Parses normalize-closing-keywords from workflow frontmatter into SafeOutputsConfig. |
| pkg/workflow/imports.go | Merges NormalizeClosingKeywords from imported configs when unset in the top-level config. |
| pkg/workflow/safe_outputs_config_generation.go | Emits normalize_closing_keywords into generated safe-outputs config.json when enabled. |
| pkg/workflow/safe_outputs_config_generation_test.go | Adds test coverage for config.json generation of normalize_closing_keywords. |
| pkg/workflow/safe_outputs_fix_test.go | Extends merge tests to cover NormalizeClosingKeywords import precedence. |
| actions/setup/js/collect_ndjson_output.cjs | Reads normalize_closing_keywords from config.json and passes it into output-type validation/normalization. |
| actions/setup/js/safe_output_type_validator.cjs | Implements normalization that removes backticks around recognized closing-keyword references for selected types’ body fields. |
| actions/setup/js/safe_output_type_validator.test.cjs | Adds JS tests verifying enabled/disabled behavior and non-interference with unrelated inline code. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 2
| // normalize-closing-keywords: when enabled, strip backticks around recognized | ||
| // issue-closing keyword references (e.g., Closes `#123`) during ingestion. | ||
| if data.SafeOutputs.NormalizeClosingKeywords != nil && *data.SafeOutputs.NormalizeClosingKeywords { | ||
| safeOutputsConfig["normalize_closing_keywords"] = true | ||
| } |
| normalizeIssueClosingKeywords = expectedOutputTypes.normalize_closing_keywords === true; | ||
| // Remove global config keys so they are not treated as valid output types | ||
| delete expectedOutputTypes.max_bot_mentions; | ||
| delete expectedOutputTypes.normalize_closing_keywords; |
| * @param {string} content | ||
| * @returns {string} | ||
| */ | ||
| function normalizeIssueClosingKeywordBackticks(content) { |
There was a problem hiding this comment.
@copilot add dedicated unit tests + fuzzer tests
There was a problem hiding this comment.
Added dedicated unit tests and fuzz-style invariant tests for closing-keyword normalization in actions/setup/js/safe_output_type_validator.test.cjs, including full keyword coverage, scope checks, idempotence, and adversarial backtick-heavy inputs. Addressed in commit fb9f9c6.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #36701 does not have the 'implementation' label and has 56 new lines of code in business logic directories (≤100 threshold). |
|
|
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
@copilot normalize-closing-keywords should be specified on each safe-outputs separately safe-outputs.normalize-closing-keywords # NO |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
![github-actions[bot]](https://github.com/_gh/avatars/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 894.1K
|
|
||
| it("should not modify unrelated backticked text when normalization is enabled", async () => { | ||
| const { validateItem } = await import("./safe_output_type_validator.cjs"); | ||
|
|
There was a problem hiding this comment.
[/tdd] No test covers pattern 3 — the \keyword` #ref` variant (keyword backticked, reference bare). All four ordered patterns exist in the implementation but only three have corresponding test cases.
💡 Suggested test to add
it("should normalize a backticked keyword with a bare reference when enabled", async () => {
const { validateItem } = await import("./safe_output_type_validator.cjs");
const result = validateItem(
{ type: "add_comment", body: "`Closes` #456" },
"add_comment", 1,
{ normalizeIssueClosingKeywords: true }
);
expect(result.isValid).toBe(true);
expect(result.normalizedItem.body).toContain("Closes #456");
expect(result.normalizedItem.body).not.toContain("`Closes`");
});Without this, ISSUE_CLOSING_KEYWORD_BACKTICK_PATTERN (pattern 3) has no regression protection.
| // https://docs.github.com/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue | ||
| const ISSUE_CLOSING_KEYWORDS = "fix|fixes|fixed|close|closes|closed|resolve|resolves|resolved"; | ||
| const ISSUE_REFERENCE_PATTERN = "(?:[a-zA-Z0-9_.-]+\\/[a-zA-Z0-9_.-]+)?#\\d+"; | ||
| const ISSUE_CLOSING_WHOLE_SPAN_PATTERN = new RegExp(`\`(\\b(?:${ISSUE_CLOSING_KEYWORDS})\\b\\s+${ISSUE_REFERENCE_PATTERN})\``, "gi"); |
There was a problem hiding this comment.
[/grill-with-docs] NORMALIZE_CLOSER_BODY_TYPES excludes submit_pull_request_review, which also has a body field that agents populate. Is this intentional? If so, a brief inline comment explaining the scope decision would help future maintainers understand why review bodies are excluded.
💡 Context
If submit_pull_request_review bodies can also contain backticked closing keywords (e.g., an agent summarising a review and closing a related issue), omitting it means the normalization silently doesn't fire for that type. If that's a deliberate call (e.g., review bodies rarely carry closing keywords), document it:
// submit_pull_request_review intentionally excluded: review bodies rarely carry issue-closing keywords
const NORMALIZE_CLOSER_BODY_TYPES = new Set(["create_issue", "add_comment", "create_pull_request"]);| // Remove global config keys so they are not treated as valid output types | ||
| delete expectedOutputTypes.max_bot_mentions; | ||
| delete expectedOutputTypes.normalize_closing_keywords; | ||
| } catch (error) { |
There was a problem hiding this comment.
[/tdd] The error path in the config-parsing catch block leaves normalizeIssueClosingKeywords = false silently. This is the correct safe default, but there's no test asserting this behavior. A config-parse failure could make an operator think normalization is running when it isn't.
💡 Suggestion
Either add a comment to the catch block to make the intent explicit:
} catch (error) {
// normalizeIssueClosingKeywords stays false — safe default on config error
const errorMsg = getErrorMessage(error);
...
}Or, if the test harness allows, add a test that passes malformed config and asserts that normalization does not run.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (11 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §26909551577
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 11 new tests verify observable behavioral contracts with strong edge-case coverage across both JavaScript (vitest) and Go unit tests.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in 7605b9a. |
|
@copilot review all comments and apply copilot-review skill |
Reviewed all in-scope comments with the copilot-review workflow. The previously raised config-collision and ingestion concerns are already addressed by the per-output move in |
|
@copilot Lint js |
GitHub does not parse closing keywords inside code spans, so outputs like
Closes \#123`` silently fail to auto-close issues. This PR adds a deterministic, opt-in normalization path that removes backticks only for recognized closing-keyword references, leaving unrelated inline code intact.Ingestion/config plumbing
normalize-closing-keywordson:safe-outputs.create-issuesafe-outputs.add-commentsafe-outputs.create-pull-requestnormalize_closing_keywordsonly within those output-type config objects in safe-outputsconfig.json.Deterministic normalization scope
safe_output_type_validator.cjsforbodyfields of:create_pull_requestcreate_issueadd_commentRepresentative behavior