Skip to content

ci: trim incident history from ci.yml comments - #781

Open
tylerkron wants to merge 4 commits into
mainfrom
cursor/trim-ci-comments-b27b
Open

tylerkron wants to merge 4 commits into
mainfrom
cursor/trim-ci-comments-b27b

Conversation

@tylerkron

@tylerkron tylerkron commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

ci.yml's comments had grown into incident history (#444/#445, #635, #689, #641), plus a second copy of the ApiCompat essay that already lives in CONTRIBUTING.md ("The published package is the second opinion"). That buried the few constraints that actually keep the workflow correct. This is a comment-only trim that keeps each constraint and its reason, in one or two lines.

Removed:

  • the #444 / #445 narration on the push backstop
  • "until now the first machine…" (#635)
  • #689 (on the test step and the coverage mv)
  • #641 on the coverage threshold
  • the duplicated ApiCompat write-up (#636 two-layer guard, narrower-not-stricter baseline). CONTRIBUTING.md on main has all of it.

Each non-obvious setting still says why it exists, so a maintainer won't "fix" it back:

  • merge_group is the queue gate. push on main is the backstop for changes that skip the queue (a ruleset bypass, an admin push).
  • Only pull_request runs are cancelled. merge_group and push runs are never redundant.
  • The baseline-drift check is deliberately a failure, not a warning: an out-of-date baseline silently narrows ApiCompat. It goes red after every release until the bump lands. (Re-added in review: the first cut had dropped this.)
  • The pack step is the second half of the public-API guard: it compares the package to the nuget.org release, which no PR can edit. RS0016/RS0017 only check the PR-editable PublicAPI.*.txt. (Re-added in review, with a pointer to CONTRIBUTING.md.)
  • The pack step leaves out --no-build on purpose, because that flag skips the ApiCompat targets. release.yml packs with --no-build and so isn't this gate.
  • dotnet pack defaults to Release, the configuration the Build step compiled (ci: build and test Release in CI #774), so it still runs the build targets and ApiCompat but skips recompiling Daqifi.Core. I checked this locally: after dotnet build -c Release, the pack log shows Skipping target "CoreCompile" for both TFMs, and ValidatePackageTask still runs.
  • Prereleases must not become the baseline. Filtering them out also keeps the baseline a plain three-part version.
  • Coverage must never fail the job. The mv is guarded because the step runs under set -e.

No workflow logic, matrix, run: content, action refs, timeouts or pack configuration changed. benchmarks.yml is untouched.

Merge note

#774 (Release build/test) and #768 (SHA pins, build timeout) have both landed on main, and this branch has origin/main merged in. The only conflict was the pack-step comment #774 rewrote. I resolved it by keeping main's -c Release run: lines and SHA pins and trimming the comment to the wording above. Against current main, the diff is still comments-only in ci.yml.

Test plan

🤖 Generated with Claude Code

Co-authored-by: Tyler Kron <tylerkron@gmail.com>
@tylerkron
tylerkron requested a review from a team as a code owner September 23, 2026 10:38
@qodo-code-review

qodo-code-review Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit 70f4475 🧠 Deep

Results up to commit 4d9be30 🚀 Fast


No changes from previous review

Results up to commit 920578f ⏭️ Skipped


No changes from previous review

Results up to commit 5b73629 ⏭️ Skipped


No changes from previous review

Grey Divider

Qodo Logo

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Trim incident history from CI workflow comments

📝 Documentation 🕐 Less than 5 minutes

Grey Divider

AI Description

• Removes historical incident narration from CI workflow comments.
• Retains concise explanations of constraints required for correct workflow behavior.
High-Level Assessment

The current approach is optimal: keep essential operational constraints beside the affected workflow steps while removing historical details already documented elsewhere. Moving all explanations out of the workflow would reduce valuable context for future maintainers.

Files changed (1) +25 / -62

Documentation (1) +25 / -62
ci.ymlCondense CI workflow guidance +25/-62

Condense CI workflow guidance

• Removes incident-specific history and duplicate explanations from workflow comments. Preserves concise rationale for merge-queue backstops, cancellation behavior, API compatibility validation, and non-blocking coverage reporting without changing workflow logic.

.github/workflows/ci.yml

The trim dropped the only ci.yml text saying the baseline-drift check is a
failure on purpose (it goes red after every release until the bump lands) and
what the pack step guards that RS0016/RS0017 cannot. Both are settings a
maintainer would plausibly "fix"; restore a short reason for each, pointing at
CONTRIBUTING.md for the full write-up.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 920578f

The comment said pack recompiles rather than "reusing the Debug output
above", which goes stale once #774 builds in Release. State only the durable
facts: pack defaults to Release and, without --no-build, runs the build itself,
recompiling only when the Release output is out of date.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 5b73629

@tylerkron

Copy link
Copy Markdown
Contributor Author

Qodo-clean, CI green — ready for review

@tylerkron
tylerkron added this pull request to the merge queue Sep 27, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Sep 27, 2026
Resolves the pack-step comment conflict with #774: keep main's -c Release
run: lines and #768's SHA pins, and state that pack reuses the Release build
(skipping the compile) while still running ApiCompat.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 70f4475

@tylerkron

Copy link
Copy Markdown
Contributor Author

Qodo-clean, CI green — ready for review

@tylerkron tylerkron mentioned this pull request Oct 1, 2026
1 task done
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.

2 participants