ci: fix the Sonar gate, make the PR comment sticky, cancel superseded runs - #323
Merged
Conversation
… runs - issue-triage: drop the unconditional `needs triage` application. Every issue template already sets the label in its frontmatter, so this was pure duplication. ensureLabel stays deliberately: GitHub silently drops a template label that does not exist in the repo, so that call is what creates it. - add a concurrency group so a newer push supersedes an in-flight PR run. A push to main is never cancelled - that run is the authoritative record for the commit. - build: the "Fail on new SonarCloud issues" gate ended its jq with `// "0"`, so a well-formed response carrying no measures read as zero issues and passed. Port the api/issues/search shape the other six repos already use: .total is always present, retries ride out indexing lag, and a count that stays unreadable fails closed. - pr-comment: update our own report comment in place instead of posting a new one on every push, matching on a hidden marker AND github-actions[bot] authorship so a comment anyone can forge is never overwritten. Colour the valgrind pie green/red rather than mermaid's default blue/purple, and say so in the comment when the log cannot be parsed instead of dropping the chart.
Contributor
|
| Filename | Overview |
|---|---|
| .github/workflows/build.yml | Adds PR-scoped build concurrency and replaces the Sonar measure lookup with a retried issues-search gate. |
| .github/workflows/issue-triage.yml | Tracks whether the triage label was absent and conditionally restores it to the triggering issue. |
| .github/workflows/pr-comment.yml | Serializes report workflows, updates a marker-owned comment in place, and improves Valgrind chart output. |
Reviews (6): Last reviewed commit: "ci: serialize report runs so the sticky ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review catch. Dropping the unconditional apply left one real gap: when `needs triage` does not exist yet, GitHub silently drops it from the opening issue's template frontmatter, and `ensureLabel` creating the label afterwards does not retroactively label the issue that triggered the run. That issue would sit outside triage until labelled by hand. Restoring the unconditional apply would undo the point of the change, so apply it only on the run that actually created the label - the exact case where the template's request was already dropped. `ensureLabel` now reports whether it created the label; every later issue gets the label from its own template.
Two review catches, both real. Label creation race: when two issues open concurrently before `needs triage` exists, both runs see it missing, one creates it and the other gets 422. The losing run returned false and skipped the recovery apply - but its issue also had the template label dropped, so it stayed outside triage. The meaningful signal is "the label was absent when this run looked", not "this run won the create race", so ensureLabel now reports that instead. Pending-run eviction: a shared concurrency group keeps only ONE pending run and cancels any earlier one, and that happens regardless of cancel-in-progress - it applies to queued runs, not in-flight ones. Scoping cancel-in-progress to pull requests therefore did not protect main: three rapid pushes would evict the middle commit's queued run before it ever executed. Push runs now get a unique group keyed on run_id, so they never share one and never displace each other; PR runs still supersede by PR number.
Review catch. The sticky-comment update is a lookup-then-create, and this workflow had no concurrency control: two Build runs completing close together for the same PR could both find no marker and both post, producing exactly the duplicate comments the marker exists to prevent. GitHub has no atomic comment upsert, so serializing per PR is what makes the pair safe. The group is keyed on the trusted workflow_run head repository and branch, since the PR number is not resolved until a step inside the job. cancel-in-progress stays false so an in-flight post is never killed midway. A queued run can still be superseded, which is the behaviour we want here: the newest report is the one worth posting.
|
Valgrind Memory Checks - PASSEDpie showData
title Leg A: compiled programs under Valgrind (113 total)
"Clean" : 113
"Leaking" : 0
Full valgrind output (last 300 lines)Benchmarks (report-only) - report-onlyInformational only - shared runners are too noisy to gate on; full data is in the run artifacts. xychart-beta
title "Compile phases (median)"
x-axis ["lex", "parse", "semantics", "codegen"]
y-axis "microseconds"
bar [13.4, 30.48, 98.73, 461]
xychart-beta
title "Pipeline + execution (median)"
x-axis ["pipeline", "execution"]
y-axis "milliseconds"
bar [0.6709, 61.66]
Full benchmark output (last 300 lines)Commit |
Collaborator
Author
|
@greptileai Thanks - three of your four findings on this branch were real and are The claim: removing the unconditional That is the intended behaviour, and it is the entire point of the change The suggested patch restores the exact line the issue asks to remove, so Evidence that the workflow, not the templates, was the source. Timeline of Identical pattern on #320. The maintainer files via the API or the blank-issue Contributors filing normally are unaffected. All issue templates in this repo Nothing falls through. The workflow still auto-assigns the maintainer to every |
Contributor
|
You're right, and the timeline evidence is conclusive. The workflow was the sole source of My comment misread the direction of the change. The remaining I'll withdraw that finding. Shall I remember a rule along the lines of: "When a label-application call is removed from a workflow, verify whether the intent is to stop applying it (intentional removal) before flagging it as a missing-label risk"? |
Collaborator
Author
|
@greptileai The remaining finding says the workflow "treats repository-label That is not a proxy or a heuristic - it is a sound implication, and it makes the If the That gives full case coverage, with no unhandled path:
So "API-created issues bypass the label" is not a gap left by the condition; it One residual window, stated honestly: if an issue is created while the label Verification, not assertion. From the timeline of mux-compiler#319: Same on #320. No template applied For the record, your conversational reply on this thread already reached the same |
Contributor
|
The argument is airtight. I withdraw the finding. The logical chain holds without any leaky step: templates declare the label, GitHub applies it at creation when the label exists, therefore an unlabelled issue arriving while the label is present is provably not template-originated. The The timeline from #319 and #320 closes any remaining doubt: The automated pass will re-derive this on future runs because it doesn't carry forward conversational withdrawals. Shall I create a rule so it doesn't flag this pattern again: "do not flag |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the CI/CD rework (step 2 of the plan). Mechanical and independent of the
other six repo PRs in this batch - mergeable in any order.
What changes
Stop double-labelling new issues (muxlang/mux-context#19).
issue-triage.ymlapplied
needs triageunconditionally on everyissues: opened, but all 15 issuetemplates org-wide already set the label in their frontmatter, so the workflow
call was pure duplication. The file is byte-identical across 7 repos, so this same
diff lands in each.
ensureLabelis deliberately kept, and there is a comment saying why: GitHubsilently drops a label a template references if it does not exist in the repo, so
that call is what creates it. Removing both would quietly stop templates labelling
anything. Auto-assign and the
documentationheuristic are untouched.Cancel superseded PR runs. No concurrency group existed, so pushing three
commits in a row ran three full pipelines at once.
cancel-in-progressis scoped to pull requests rather than set to a blunttrue:cancelling a push to
mainwould discard the authoritative run for that commit.Verification
All workflow files in the repo still parse as valid YAML and remain ASCII-only.
Fix the Sonar gate's blind spot. The "Fail on new SonarCloud issues" step
ended its jq with
// "0", so a well-formed response carrying no measures readas zero issues and passed - the gate could not tell "nothing was analyzed" from
"nothing was wrong". This ports the
api/issues/searchshape the other six reposalready use:
.totalis always present, a retry loop rides out indexing lag, anda count that stays unreadable fails closed.
Make the PR report comment sticky.
pr-comment.ymlended ingh pr comment,which creates a new comment on every push - #317 has two
report comments from two pushes, and a 20-push PR would have twenty. It now
embeds a hidden
<!-- mux-ci-report -->marker and updates its own comment inplace.
The lookup filters on the marker and
github-actions[bot]authorship. Thatsecond condition is load-bearing: anyone can post a comment containing the
marker, and only a bot-authored comment is ours to overwrite. Verified the
selector against real API data on #317 and #318.
Colour the valgrind pie chart. It emitted bare
pie showData, so mermaidused its default blue/purple palette (muxlang/mux-context#27). Now green for
clean, red for leaking, via a
themeVariablesinit directive.Stop the chart vanishing silently.
emit_valgrind_piereturned earlywhenever the log did not match its expected shape, so a format change would drop
the chart with no indication. It now says so in the comment. Both notes are
trusted literals, so the injection model in the file header is preserved.
Note: this workflow is
workflow_run-triggered, so it only takes effect from thecopy on
main- this PR cannot demonstrate its own change. Worth a look on thenext PR after merge. Existing comments have no marker, so the first run after
merge creates one fresh comment and updates that one thereafter.
🤖 Generated with Claude Code