Name a Recovery a Dispatch Can Actually Perform in D4.5 - #1213
Merged
Conversation
CodeRabbit found on #1203 that D4.5 names a recovery the publisher forbids. It said a failed package push is recovered "by re-dispatching the same commit", and workflow_dispatch accepts a branch or tag ref rather than a commit SHA. publish-release.yml triggers on workflow_dispatch alone, its assert-dispatch-ref job rejects any ref_name that is not main or develop, and the publish job passes ref: github.sha, the dispatch-time tip. A maintainer following that sentence runs gh workflow run --ref <sha>, gets HTTP 422, and finds the release tag rejected too. The finding asked for the bound in all four D4.5 copies. WORKFLOW.md stated it in its next sentence and still opened with the same unperformable phrasing, so both surfaces move here rather than only the condensed one. WORKFLOW.md now says a re-dispatch "while the branch tip is still the commit whose push failed", which is also the accurate half, since the build succeeded and only the push failed, and adds that the dispatch names the branch rather than the commit. The condensed catalog carries the limit and the harm together, that a re-dispatch after the tip has moved publishes a further version and leaves the first release without its package, with the git-height cause that makes it deterministic rather than merely risky. version.json declares no pathFilters, so any commit advances height. Both come from this promotion. #1198 added the failed-push paragraph to WORKFLOW.md with its caveat and added the condensed restatement without it, so the authority and the catalog disagreed from the moment they landed. Whole-unit passes over the two changed units settled the wording, rejecting an earlier revision at each of an operation the publisher forbids, a relative clause attached to cleanup rather than to the re-dispatch, a split correlative, an antecedent-less demonstrative, and a garden path off the branch tip. The pre-existing defects those passes found in the same units are filed as #1200 and #1212.
PR Summary by QodoClarify D4.5 Package Push Recovery Constraints
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change clarifies D4.5 package-push recovery. An unchanged branch tip repairs the existing release. A moved tip creates a newer version. Related workflow references, source metadata, and canonical review records are updated. ChangesRelease and publish contract
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to The PR corrects recovery guidance so failed package pushes are re-dispatched from the unchanged branch tip rather than an unsupported commit reference. No actionable merge-blocking risk remains; it is merge-ready after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The updated D4.5 recovery sentence in WORKFLOW.md can still be misread as “dispatch a commit SHA,” so the actionable branch-ref requirement should be stated more explicitly to fully resolve the original finding.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the D4.5 documentation contract so the “failed package push” recovery describes a workflow_dispatch rerun that a maintainer can actually perform, and propagates the corrected wording across the carried guarantee catalogs and canonical-review ledger.
Changes:
- Refines
WORKFLOW.mdD4.5 recovery wording to bound re-dispatch recovery to when the branch tip still points at the failed-push commit. - Updates the D4.5 condensed restatement in the workflow CI contract’s
d-guarantees.mdacross source and generated distributions. - Refreshes
reports/canonical-review.jsondigests/findings metadata for the touched units, plus the fleet-skills source digest.
File summaries
| File | Description |
|---|---|
| WORKFLOW.md | Adjusts D4.5 prose to describe a dispatch recovery that depends on the branch tip not advancing. |
| .agents/skills/workflow-ci-contract/references/d-guarantees.md | Updates the condensed D4.5 guarantee summary to match the corrected recovery bound. |
| .github/skills/workflow-ci-contract/references/d-guarantees.md | Regenerates the distributed copy of the condensed D4.5 guarantee. |
| .claude-plugin/fleet-skills/skills/workflow-ci-contract/references/d-guarantees.md | Regenerates the plugin-distributed copy of the condensed D4.5 guarantee. |
| .claude-plugin/fleet-skills/.source-digest | Updates the fleet-skills source digest after regeneration. |
| reports/canonical-review.json | Updates canonical-review digests/stamps/findings counts for the modified units. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot found on #1213 that the recovery wording still foregrounds "commit" and relegates the actionable constraint, that a dispatch must name a branch ref rather than a commit SHA, to a trailing clause, where it can reintroduce the operator mistake the change exists to prevent. The finding is right and two local passes had already flagged the same placement as a non-blocking style note. Classifying it that way twice was the error. The constraint is what stops an operator forming the plan at all, so its position is a correctness property of the item rather than a matter of flow. The passage now runs constraint first, consequence after: a dispatch names a branch, main or develop per D2.3, and never a commit, so what it builds is that branch's tip at dispatch time, and a re-dispatch therefore repairs the failed version only while the tip is still the commit whose push failed. Copilot's suggested wording used a semicolon, which the fleet's prose rules forbid, so the content is taken and the punctuation is not.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The updated D4.5 language is consistent with the actual dispatch/ref gate behavior and the change is correctly propagated to all carried/distributed copies and review metadata.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
ptr727
added a commit
that referenced
this pull request
Sep 2, 2026
…orkflows Falsify (#1203) Closes #1152. Closes #1174. Closes #1175. Closes #1176. Closes #1177. Closes #1178. Promotes two commits. The first settles seven `WORKFLOW.md` section 4 contract claims that the workflow files and stubs they govern contradict. The second fixes what this promotion's own review round then found. ## What is settled **A caller cannot address a callee's job.** D1.2 named `validate-task.yml`'s internal job, and a `workflow_call` callee's jobs resolve nowhere in a caller, so the name an aggregator `needs:` is always the caller's own `validate` stub job. The task detects the tree rather than the language, so a non-.NET repo calls the same one instead of replacing it. **The aggregator's dependency list was wrong in two directions.** D1.2 claimed `smoke-build` needs the validation job, an edge no shipped stub carries, and section 6 repeated it twice. D1.5 required a `needs:` on the `changes` job unconditionally, which a no-build repo does not have, so its aggregator would fail to load. D1.5 also never stated `if: always()`, without which the aggregator inherits `success()` and is skipped the moment a dependency fails, which GitHub reads as a satisfied required check. **The paths filter was described as a negation.** Verified against picomatch: a lone `!.github/workflows/**` matches every other path, so a docs-only pull request marks the target and smoke-builds, the opposite of scenario S2. The stub now carries an inclusion list naming the target's own paths, satisfying S2 and S3 together. **The no-asset list named the wrong third shape.** D4.3 disagreed with the release task's own header, which named "NuGet symbols-only" while `nuget-build-default` uploads a package-carrying asset. The third shape is source-only. **A rejected token exchange can leave a release for a version the registry never received.** D4.5 promised nothing partial ships and covered only a failed build. Moving the package push into the publishing repository's own job put it after `github-release`. The item names that case and bounds its recovery, since a re-dispatch after the branch tip has moved builds the new tip, NBGV deriving the version from git height. **A cleanup step skipped exactly the failure it was written for.** D5.2 described a delete that runs unconditionally while both publish stubs carried the step with no `if:`, so it inherited `success()`. Both now carry `if: ${{ !cancelled() && steps.download.outcome == 'success' }}` with an `id:` on the download. **Adopting the release chain needs the registry policy repointed.** A repository that worked around the earlier `HTTP 401` by pointing its nuget.org policy at `build-release-task.yml` now mismatches from the other direction, and the adopter-owes list omitted it. ## What this promotion's own review then fixed #1198 changed section 4's preamble and left three other surfaces stating the framing it replaced. #1209 settles them, and deliberately asserts no framing in their place, since D9's items and D6.2 state properties of committed source rather than any output a run produces: - `WORKFLOW.md`'s file preamble now routes to section 4's preamble rather than restating it. - The `workflow-ci-contract` skill's `description:` names the guarantees and stops. - Its pointer sentence names what each reference condenses, which also supplies an antecedent it lacked, and stops describing `references/test-methodology.md` as the audit procedure when that file calls itself an index into `WORKFLOW.md` 5A. #1209 also adds `global.json` to both paths-filter lists in `docs/reusable-workflows.md`. D1.6 requires that file of any repository whose test project is MTP-based, and an adopter copying the example verbatim skipped the smoke build on a change to it. #1213 then settles a defect #1198 introduced on both of its D4.5 surfaces at once. The item named a recovery the publisher forbids, "re-dispatching the same commit", where `workflow_dispatch` accepts a branch or tag ref rather than a commit SHA, `assert-dispatch-ref` gates the ref to `main` or `develop`, and the publish job passes `ref: ${{ github.sha }}`. A maintainer following it ran `gh workflow run --ref <sha>`, got `HTTP 422`, and found the release tag rejected too. `WORKFLOW.md` now leads with the constraint rather than trailing it, since a constraint an operator meets after forming the plan does not stop the plan, and the condensed catalog carries the bound and the harm together with the git-height cause that makes it deterministic. ## Review Eight whole-unit rounds on #1198 across thirteen carried canonical units. Two review rounds on #1203 raising eight findings, and two on #1209. Of the eight raised here, three were fixed by #1209, one deferred behind a filed issue, and four declined with evidence in their threads: the `WORKFLOW.md`-duplicates-`GOVERNANCE.md` finding against the "Canonical scope" blockquote that declares the overlap and its precedence rule, the comment-length finding against the rule's own "several sentences is several lines" clause and twenty-plus shipped workflows carrying the shape, the failed-push-recovery finding against #1177's specified shape and the cost D5.2 and D4.5 now state, and the S9 artifact finding against D5.2's two artifact families with their two different gates. ## Deliberately not in this promotion Whole-unit passes found pre-existing defects in every unit they read, and the second review round on this PR raised seven more that are pre-existing on `main` rather than introduced here. Each is filed rather than swept in. - #1199 through #1202, from #1198's own round. #1199 is the live one, where `validate-task.yml` runs `pytest --cov-report=xml` with no `--cov`, so it collects nothing and writes no `coverage.xml` while the upload step stays green. CodeRabbit raised it independently here. - #1204, S9's narrowing leaves D4.4's dispatch-refresh leg covered by no scenario in exactly the repositories where a repeated dispatch is the ordinary way to re-cut a release. - #1205, #1206, #1207, #1208 and #1212, claims in `WORKFLOW.md`'s preamble, the skill description, its Core Behavioral Spine, and the condensed D4 catalog that the documents themselves falsify. - #1210, canonical-review entries whose `hubCommit` resolves nowhere on the remote. - #1211, the documented pull request aggregator accepts a `skipped` validator where this repository's own workflow requires `success`, so the canonical stub is weaker than the reference implementation it documents. - #1214, D4.5 and D5.2 together imply a failed push is unrecoverable once the branch tip moves, and neither considers **Re-run all jobs**, which replays at the run's original `github.sha`. Settling it needs a real publish against a real registry, which is the maintainer's to run. Two findings from the second round were declined with evidence rather than filed: the `enable_<target>` forwarding finding, which is false for the single-target example the document shows and whose multi-target pattern is documented in four places, and the S7 cleanup finding, which section 5's own N/A rule and the per-type walkthroughs already resolve. Sixteen carried canonical units moved across the two commits, so this re-vendors fleet-wide and the audit drift wave that follows is the mechanism working. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Workflow Improvements** - Manual release dispatches now refresh existing releases instead of skipping them. - Scheduled and push reruns avoid unnecessary republishing. - Validation, dependency handling, release gating, and partial-failure behavior are more consistent. - Artifact cleanup now occurs only when artifacts are no longer needed. - **Documentation** - Clarified release assets, trusted publishing requirements, workflow paths, and rerun behavior. - Expanded guidance for NuGet, PyPI, Docker, and source-only release scenarios. - Improved workflow governance and review guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
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.
One reviewer finding from #1203, the
develop -> mainpromotion PR, fixed at its source in both places it appears. A promotion PR's head isdevelop, so it could not be pushed there directly.What the finding was
D4.5 named a recovery the publisher forbids. It said a failed package push is recovered "by re-dispatching the same commit", and
workflow_dispatchaccepts a branch or tag ref rather than a commit SHA.publish-release.ymltriggers onworkflow_dispatchalone, itsassert-dispatch-refjob rejects anyref_namethat is notmainordevelop, and the publish job passesref: ${{ github.sha }}, the dispatch-time tip.A maintainer with a failed NuGet push follows that sentence, runs
gh workflow run publish-release.yml --ref <failed-sha>, getsHTTP 422 No ref found, falls back to the release tag, and is rejected again by the ref gate.Why both copies moved
CodeRabbit asked for the bound in all four D4.5 copies. A first pass at this fixed the three condensed ones and left
WORKFLOW.md, on the reading that it already stated the limit in its next sentence. It does, and it still opened with the same unperformable phrasing, so a reader routed to the authority for exact wording landed on the less accurate of the two texts.WORKFLOW.mdnow says a re-dispatch "while the branch tip is still the commit whose push failed", which is also the accurate half, since the build succeeded and only the push failed, and adds that the dispatch names the branch rather than the commit.The condensed catalog carries the limit and the harm together, that a re-dispatch after the tip has moved publishes a further version and leaves the first release without its package, with the git-height cause that makes it deterministic rather than merely risky.
version.jsondeclares nopathFilters, so any commit advances height.Provenance
Both come from the promotion this fixes. #1198 added the failed-push paragraph to
WORKFLOW.mdwith its caveat and added the condensed restatement without it, so the authority and the catalog disagreed from the moment they landed.Review
Whole-unit passes over the two changed units settled the wording, and the process is worth recording because the sentence took several attempts. Each earlier revision was rejected for a distinct, real defect:
cleanuprather than to the re-dispatchrather than by cleanupread as attaching torepairsa later oneread offthe branch tipThe shipped wording is a reviewer-supplied split that carries none of them.
Deliberately not here
The same passes found pre-existing defects in both units, each filed rather than swept in:
if:is actually narrower, addinginputs.expect_release_assets &&.success.Verification
Every gate
OPERATIONS.md"Local Verification" names, run at the head commit:build_dist.py --check,canonical_review.py checkandreport --check,repo_gate.py,prose_lint.py,spec/validate.py, andmarkdownlint-cli2on both changed Markdown files. All green.Two changed carried units, each covered by a recorded whole-unit pass, and the diff receipt recorded against the shipping content. Both ledger entries name a commit that resolves on the remote.
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores