add-comment: support hide-older-comments.match for exact multi-workflow comment minimization#37977
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>
hide-older-comments.match for exact multi-workflow comment minimization
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR extends the add-comment safe-output handler so hide-older-comments can minimize older comments produced by multiple workflow IDs (exact workflow-id marker matches), not just the current workflow ID. It does this by introducing an object-form config (enabled + match) while keeping the existing boolean/templatable form for backward compatibility.
Changes:
- Add Go-side config preprocessing to support
hide-older-comments: { enabled, match }, extractingmatchinto compiled handler config. - Update the JS
add_commenthandler to compute an effective set of workflow IDs (current + configured matches) and hide comments matching any of them. - Expand the JSON schema and add targeted Go/JS test coverage for object-form config and multi-ID hiding.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_handler_registry.go | Propagates hide_older_comments_match into the handler config JSON passed to the JS runtime. |
| pkg/workflow/add_comment.go | Adds parsing/preprocessing for object-form hide-older-comments and stores extracted match IDs in config. |
| pkg/workflow/add_comment_target_repo_test.go | Adds Go test coverage for object-form parsing/defaults and match extraction. |
| pkg/parser/schemas/main_workflow_schema.json | Extends hide-older-comments schema to oneOf (templatable boolean or object with enabled/match). |
| actions/setup/js/add_comment.test.cjs | Adds JS tests validating multi-ID exact-match hiding and enabled: false behavior. |
| actions/setup/js/add_comment.cjs | Implements multi-ID workflow marker matching in hide/minimize logic and object-form config support. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 3
| @@ -17,11 +19,12 @@ type AddCommentsConfig struct { | |||
| TargetRepoSlug string `yaml:"target-repo,omitempty"` // Target repository in format "owner/repo" for cross-repository comments | |||
| AllowedRepos []string `yaml:"allowed-repos,omitempty"` // List of additional repositories that comments can be added to (additionally to the target-repo) | |||
| HideOlderComments *string `yaml:"hide-older-comments,omitempty"` // When true, minimizes/hides all previous comments from the same workflow before creating the new comment | |||
| AllowedReasons []string `yaml:"allowed-reasons,omitempty"` // List of allowed reasons for hiding older comments (default: all reasons allowed) | |||
| Issues *bool `yaml:"issues,omitempty"` // When false, excludes issues:write permission and issues from event condition. Default (nil or true) includes issues:write. | |||
| PullRequests *bool `yaml:"pull-requests,omitempty"` // When false, excludes pull-requests:write permission and PRs from event condition. Default (nil or true) includes pull-requests:write. | |||
| Discussions *bool `yaml:"discussions,omitempty"` // When false, excludes discussions:write permission. Default (nil or true) includes discussions:write. | |||
| Footer *string `yaml:"footer,omitempty"` // Controls whether AI-generated footer is added. When false, visible footer is omitted but XML markers are kept. | |||
| HideOlderCommentsMatch []string `yaml:"hide-older-comments-match,omitempty"` | |||
| AllowedReasons []string `yaml:"allowed-reasons,omitempty"` // List of allowed reasons for hiding older comments (default: all reasons allowed) | |||
| * @param {number} issueNumber - Issue/PR number | ||
| * @param {string} workflowId - Workflow ID to search for | ||
| * @param {string[]} workflowIds - Workflow IDs to search for | ||
| * @returns {Promise<Array<{id: number, node_id: string, body: string}>>} | ||
| */ | ||
| async function findCommentsWithTrackerId(github, owner, repo, issueNumber, workflowId) { | ||
| async function findCommentsWithTrackerId(github, owner, repo, issueNumber, workflowIds) { |
| * @param {number} discussionNumber - Discussion number | ||
| * @param {string} workflowId - Workflow ID to search for | ||
| * @param {string[]} workflowIds - Workflow IDs to search for | ||
| * @returns {Promise<Array<{id: string, body: string}>>} | ||
| */ | ||
| async function findDiscussionCommentsWithTrackerId(github, owner, repo, discussionNumber, workflowId) { | ||
| async function findDiscussionCommentsWithTrackerId(github, owner, repo, discussionNumber, workflowIds) { |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (4 tests analyzed)
Inflation Ratio
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: §27177131959
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 4 new test scenarios (2 JavaScript, 2 Go table rows) verify observable behavioral contracts with edge-case coverage and no guideline violations.
Skills-Based Review 🧠Applied 📋 Key Themes & HighlightsKey Themes
Positive Highlights
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 448.3 AIC · ⌖ 13.6 AIC
| const hideOlderCommentsMatch = Array.isArray(hideOlderCommentsConfig?.match) | ||
| ? normalizeWorkflowIdList(hideOlderCommentsConfig.match) | ||
| : Array.isArray(config.hide_older_comments_match) | ||
| ? normalizeWorkflowIdList(config.hide_older_comments_match) |
There was a problem hiding this comment.
[/tdd] The config.hide_older_comments_match branch (lines 405–406) — the actual compiled production path — has no test coverage. When Go compiles a workflow, the registry sends hide_older_comments_match as a flat top-level key with hide_older_comments as the string "true"; this else-branch is what runs in production. The existing tests only exercise the object-direct path (hide_older_comments is an object), leaving the live code path untested.
💡 Suggested test (compiled config form)
// mirrors what the Go registry actually sends to the JS handler
const handler = await eval(`(async () => {
${addCommentScript};
return await main({
hide_older_comments: "true",
hide_older_comments_match: ["other_workflow"]
});
})()`);
const result = await handler({ type: "add_comment", body: "compiled path test" }, {});
expect(hiddenNodeIds).toContain("IC_kwDOTest11"); // other_workflowThis mirrors the actual runtime values set by AddTemplatableBool + AddStringSlice in safe_outputs_handler_registry.go.
|
|
||
| if err := preprocessHideOlderCommentsConfig(configData, addCommentLog); err != nil { | ||
| addCommentLog.Printf("Invalid hide-older-comments configuration: %v", err) | ||
| return nil |
There was a problem hiding this comment.
[/diagnose] An invalid hide-older-comments.enabled value causes parseCommentsConfig to return nil, silently dropping the entire add-comment configuration. The error is only logged internally; the workflow author gets no compile-time diagnostic explaining why their add-comment handler disappeared.
💡 Suggestion
Consider following the existing preprocessBoolFieldAsString pattern more closely — those also return nil on error, but the enabled case is harder to diagnose because it removes the whole handler rather than one field. At minimum, upgrade the log to a user-visible error (e.g. via the compiler error collection) or add a compile step that rejects the workflow with a descriptive message like "hide-older-comments.enabled must be a boolean or ${{ expr }}".
| ids | ||
| .filter(id => typeof id === "string") | ||
| .map(id => id.trim()) | ||
| .filter(Boolean) |
There was a problem hiding this comment.
[/tdd] normalizeWorkflowIdList trims whitespace and deduplicates entries, but neither behaviour is exercised by any test. A user authoring match: [" wf-a ", "wf-a"] should get ["wf-a"] — both the dedup and the trim are load-bearing for correctness.
💡 Suggested unit-level assertions
Since this is a pure function, it can be tested directly (or via a small dedicated test block without needing a full mock setup):
// trim
normalizeWorkflowIdList([" wf-a ", "wf-b "]) // → ["wf-a", "wf-b"]
// dedup
normalizeWorkflowIdList(["wf-a", "wf-a", "wf-b"]) // → ["wf-a", "wf-b"]
// empty-string / whitespace-only filtered
normalizeWorkflowIdList(["", " ", "wf-a"]) // → ["wf-a"]| } else if (workflowId) { | ||
| await hideOlderComments(githubClient, repoParts.owner, repoParts.repo, itemNumber, workflowId, isDiscussion); | ||
| } else { | ||
| const hideWorkflowIds = normalizeWorkflowIdList([workflowId, ...hideOlderCommentsMatch]); |
There was a problem hiding this comment.
[/tdd] When GH_AW_WORKFLOW_ID is not set, workflowId is "" (line 452) and is correctly filtered out by normalizeWorkflowIdList. This means hiding is driven entirely by hideOlderCommentsMatch. There is no test for this path — confirming that { match: ["target-wf"] } alone hides the right comments when the env var is absent.
💡 Suggested test
it("should hide match-list comments when GH_AW_WORKFLOW_ID is not set", async () => {
delete process.env.GH_AW_WORKFLOW_ID;
// ... setup mocks ...
const handler = await eval(`... main({ hide_older_comments: { match: ["target-wf"] } }) ...`);
const result = await handler({ type: "add_comment", body: "test" }, {});
expect(hiddenNodeIds).toContain(/* target-wf comment node */);
expect(hiddenNodeIds).not.toContain(/* unrelated comment node */);
});| PullRequests *bool `yaml:"pull-requests,omitempty"` // When false, excludes pull-requests:write permission and PRs from event condition. Default (nil or true) includes pull-requests:write. | ||
| Discussions *bool `yaml:"discussions,omitempty"` // When false, excludes discussions:write permission. Default (nil or true) includes discussions:write. | ||
| Footer *string `yaml:"footer,omitempty"` // Controls whether AI-generated footer is added. When false, visible footer is omitted but XML markers are kept. | ||
| HideOlderCommentsMatch []string `yaml:"hide-older-comments-match,omitempty"` |
There was a problem hiding this comment.
[/grill-with-docs] The yaml:"hide-older-comments-match" tag looks like a user-facing YAML key, but this field is an internal intermediate value injected by preprocessHideOlderCommentsConfig — it does not appear in the schema and cannot be set directly in workflow YAML. Future contributors may be confused about why it exists as a struct tag vs. a typed nested field.
💡 Suggested comment
// HideOlderCommentsMatch is an internal field populated by preprocessHideOlderCommentsConfig
// from the hide-older-comments.match array. Not a user-facing YAML key; the schema exposes
// it only via the hide-older-comments object form.
HideOlderCommentsMatch []string `yaml:"hide-older-comments-match,omitempty"`
|
``
|
|
``
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in |
|
@copilot review all review comments |
Reviewed the in-scope Copilot review comments against the current head. All three are already satisfied in |
Added Hide Older Comments (hide-older-comments) entry covering the new object form with match field for multi-workflow comment minimization (introduced in #37977). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Some workflows emit multiple distinct comments per run, and current
hide-older-commentsbehavior only targets one workflow ID, forcing brittle downstream cleanup. This change adds explicit exact-match workflow ID selection so older comments from multiple configured workflows can be minimized in one pass.add-commenthide logic now supports multi-ID matchingConfig model: object form for
hide-older-commentshide-older-comments.enabledhide-older-comments.match(array of workflow IDs)Compiler + handler config plumbing
hide-older-comments.hide_older_comments_match.Schema update
add-comment.hide-older-commentsschema from templatable boolean tooneOf:enabledandmatchTargeted coverage updates
enabled: falsebehavior in object form