Repository navigation
fix(xtask): harden changelog generation - #6915
Conversation
…nt PR ref command_output_with_timeout spawned git/gh/claude without an explicit stdin, so the child inherited the caller's stdin instead of getting a closed one as the previous Command::output() calls did. An interactive prompt from any of those tools (e.g. gh device auth) would now block until the timeout instead of failing immediately. Also add the missing (#6915) PR reference to the changelog fragment and rename it to sort with the other numbered fragments.
Several comments added by the changelog hardening pass split a single sentence across two lines instead of breaking at a sentence boundary, violating the repo's prose-wrapping convention.
There was a problem hiding this comment.
Commit 207ffe7a5 ("fix(xtask): close stdin on wrapped subprocesses, fix changelog fragment PR ref") has its author identity set to Claude <noreply@anthropic.com> (git log --format='%an <%ae>' 13a30adc5..HEAD).
This is exactly the case scripts/hooks/commit-msg is designed to reject even when the commit message text itself is clean, per CLAUDE.md's "No AI / Claude attribution" rule.
Fixing this requires rewriting the commit (amend/rebase) and force-pushing, which is outside the scope of an automated hardening pass — please re-author that commit (git commit --amend --author="<your name> <your email>" or an interactive rebase) and force-push before merge.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Deep-dived the changelog-generation hardening diff. Functionally sound and well-tested; the one actionable item is that a second commit picked up the same bad author-identity problem the earlier review already flagged on this thread.
Generated by Claude Code
| @@ -1,10 +1,19 @@ | |||
| use crate::common::repo_root; | |||
There was a problem hiding this comment.
A prior review comment on this thread flagged commit 207ffe7 for having its author identity set to Claude <noreply@anthropic.com>, but the branch has since grown a second commit with the same problem: 88b607a1 ("fix(xtask): join mid-sentence comment line breaks in changelog hardening") is also authored as Claude <noreply@anthropic.com> (git log --format='%an <%ae>' origin/main..HEAD). Both commits need re-authoring (interactive rebase, or amend + rebase) before merge, per CLAUDE.md's "No AI / Claude attribution" rule and the commit-msg hook that enforces it server-side.
Functional review of the diff itself came back clean. command_output_with_timeout correctly wraps the child in a process group (Unix) / job object (Windows) so a timeout kills descendants holding the output pipes open, not just the direct child — verified against command_timeout_covers_descendants_holding_output_pipes and the process-group semantics ProcessGroup::leader() implies. format_highlights_output closing off model-generated structural headings (rejecting anything after the first line that starts with #) is a real hardening: without it, a PR title crafted to prompt-inject the highlights-summarization call could smuggle a fake ## [...] heading into CHANGELOG.md verbatim. The render_changelog fix to skip the ## [Unreleased] heading itself when picking an insertion point (rather than falling back to "first heading of any kind") correctly keeps Unreleased on top for a first release with no dated heading yet — keeps_unreleased_on_top_for_first_release_without_dated_heading exercises exactly that case. The fail-closed changes (require_pr_numbers, fetch_pr_info returning Result and aborting on the first failure via collect::<Result<_,_>>()) are a deliberate behavior change from "warn and publish anyway" to "abort the release" on incomplete PR metadata — worth a maintainer's explicit sign-off since it can now hard-fail cargo xtask release in a case that previously degraded gracefully, but it matches the PR's stated fail-closed intent and is consistent with the rest of the hardening.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Automated daily review pass. Diff logic review of the changelog-hardening changes (process-tree timeout wrapper, fail-closed git/gh metadata handling, format_highlights_output heading-injection guard, and the render_changelog Unreleased-on-top fix for a first release) came back clean and matches its own new test coverage (keeps_unreleased_on_top_for_first_release_without_dated_heading, rejects_structural_headings_in_generated_highlights, command_timeout_covers_descendants_holding_output_pipes). The changelog.d/fixed/6915-changelog-generation-hardening.md fragment is present and correctly formatted (single sentence, no leading - , trailing (#6915) (@houko)), and the unrecognised-section-directory warning in warn_unrecognised_sections/scripts/check-changelog-attribution.py is untouched by this diff. Two findings left inline: the Claude-authored commit identities on this branch (still present, blocks merge per CLAUDE.md) and the fail-closed require_pr_numbers behavior change (needs explicit maintainer sign-off).
Generated by Claude Code
There was a problem hiding this comment.
Still unresolved at the current head sha (88b607a1, same commit two prior review passes already flagged): commits 207ffe7a5 and 88b607a1 on this branch both have author identity Claude <noreply@anthropic.com> (git log --format='%an <%ae>' 13a30adc5..HEAD).
This is exactly what the commit-msg hook's author-identity check rejects per CLAUDE.md's "No AI / Claude attribution" rule, even though the commit message text itself is clean.
Re-authoring requires an interactive rebase or amend-and-rebase followed by a force-push, which is outside the scope of an automated review — please re-author both commits before merge.
Generated by Claude Code
| } | ||
|
|
||
| fn extract_pr_numbers(root: &Path, git_range: &str) -> Vec<u64> { | ||
| fn require_pr_numbers(pr_numbers: Vec<u64>, git_range: &str) -> Result<Vec<u64>, String> { |
There was a problem hiding this comment.
require_pr_numbers turns an empty PR range into a hard error, so cargo xtask release now aborts entirely whenever a release range has zero #N-referencing commits (a hotfix release built from non-PR commits, a squash-merge series with no PR refs in the subject, etc.). The prior behavior printed "No PRs found in range" and still generated a changelog (with no generated PR bullets). This matches the PR's stated fail-closed intent and both the process-tree timeout and metadata-fetch fail-closed changes look correct, but flagging per CLAUDE.md's PR-hygiene guidance: a behavior change from "degrade gracefully" to "hard-fail the release" is worth an explicit maintainer sign-off before merge, since it can now block a release that previously would have succeeded.
Generated by Claude Code
There was a problem hiding this comment.
Commit authorship: two commits on this branch are both authored and committed as Claude <noreply@anthropic.com>:
88b607a13d367adbfede7dce661675a500a46043("fix(xtask): join mid-sentence comment line breaks in changelog hardening")207ffe7a589f05b734930bcf9b6092d8a03e8688("fix(xtask): close stdin on wrapped subprocesses, fix changelog fragment PR ref")
Only the base commit b209260fecca738e0f594808381566ff0b769bbc carries a human author/committer identity (Evan Hu).
This is a git var GIT_AUTHOR_IDENT / committer-identity match on Claude/Anthropic, the exact case this repo's own commit-msg hook is documented to reject (CLAUDE.md: "separately rejects a commit whose author identity ... resolves to Claude / Anthropic even when the message itself is clean").
The commit messages themselves are clean (no inline attribution text), but the author/committer fields are not, so this got past the message-only checks — most likely because these two commits landed via API/direct push rather than a local git commit that goes through scripts/hooks/commit-msg.
Per repo policy this needs a human decision on remediation (rebase with corrected author identity, or squash into the base commit under the human author) — not something to silently rewrite from an automated review pass.
Flagging per the recurring pattern seen across sibling PRs in this batch.
Generated by Claude Code
| }) { | ||
| // Insert before the first dated release heading so a leading `## [Unreleased]` section stays on top. | ||
| // Fall back to the first heading of any kind when no dated release exists yet. | ||
| // If there is no dated heading, skip the Unreleased heading itself and insert before any later custom section. |
There was a problem hiding this comment.
Minor test-coverage gap: this heading_re.find_iter(content).find(|heading| !... starts_with(UNRELEASED_HEADING)) fallback (skip the ## [Unreleased] heading itself and insert before a later non-dated custom heading) doesn't appear to be exercised by the new test keeps_unreleased_on_top_for_first_release_without_dated_heading.
That test's content is "# Changelog\n\n## [Unreleased]\n\n### Added\n\n- pending (#9) (@me)\n" — the only ## [ heading present is ## [Unreleased] itself, which this find filters out, so the Option here is None and the case actually falls through to the final else branch (plain append at end of content), not through this new find_iter branch.
The test still passes (append-at-end happens to keep Unreleased first), but it isn't testing the code added on this line.
Could you either add a fixture with a genuine non-dated, non-Unreleased heading after [Unreleased] (e.g. a legacy ## [Pre-1.0 notes] section) to actually hit this path, or fold the logic into the final else branch if it turns out this fallback is unreachable for any real CHANGELOG.md shape?
Generated by Claude Code
…e new validator format_highlights_output was inserted between the existing doc comment and generate_highlights, leaving the comment describing claude-CLI behavior attached to the wrong function.
Summary
Verification
cargo test -p xtask(133 passed)cargo clippy -p xtask --all-targets -- -D warningscargo fmt --checkgit diff --checkReport:
confirmed/xtask/src/changelog.rs.md(6 findings)