Repository navigation
feat: add changelog PR coverage test and backfill missing entries - #17
Merged
Merged
Conversation
📝 WalkthroughWalkthroughUpdates the Unreleased section of Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI runner
participant Test as changelog test
participant GH as GitHub REST API
participant Repo as Repository (CHANGELOG.md)
CI->>Test: run tests (including changelog_test)
Test->>Repo: read CHANGELOG.md
Test->>GH: GET /pulls?state=closed&base=main (paginated)
GH-->>Test: merged PR pages (numbers)
Test->>Test: extract IDs from CHANGELOG.md, compute difference
alt missing IDs found
Test-->>CI: fail with missing list
else all present or no merged PRs
Test-->>CI: pass (or skip if GH unreachable in non-CI)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/changelog_test.rs (1)
9-17: Make missing-ID assertion output deterministic.
HashSetiteration order is unstable, so failure output order can vary. Sortmissingbefore asserting to keep CI diffs predictable.Proposed fix
- let missing: Vec<_> = git_pr_ids + let mut missing: Vec<_> = git_pr_ids .iter() .filter(|id| !changelog_ids.contains(id)) - .collect(); + .copied() + .collect(); + missing.sort_unstable();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/changelog_test.rs` around lines 9 - 17, The failure message can vary because HashSet iteration order is unstable; collect the missing IDs into a Vec (the existing missing variable), sort it deterministically (e.g., sort() or sort_unstable()), and then assert on the sorted missing vector so the "CHANGELOG.md is missing PR IDs: {:?}" output is stable; update the logic around git_pr_ids, changelog_ids, and missing to sort missing before the assert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 31: Replace the typo "TODO/FFIXME" in the changelog entry with the
correct marker "TODO/FIXME" so the user-facing release notes read: "Hygiene
checks for file-size limits and tech-debt markers (TODO/FIXME) in the canonical
`just check` gate and CI workflow. (`#10`)"; locate the string "TODO/FFIXME" in
CHANGELOG.md and update it to "TODO/FIXME".
In `@tests/changelog_test.rs`:
- Around line 25-32: The function extract_git_merged_pr_ids should only consider
PR IDs from merge commits and must validate the git command succeeded; change
the git invocation to include only merges (e.g., use "git log --merges --all
--format=%s") and after .output() assert output.status.success(), bubbling
stderr on failure, then pass output.stdout through regex_find_pr_ids; apply the
same changes to the analogous function handling lines ~34-55 so both use
"--merges" and check output.status.success() (include stderr in the error
message) to avoid false positives and silent failures.
---
Nitpick comments:
In `@tests/changelog_test.rs`:
- Around line 9-17: The failure message can vary because HashSet iteration order
is unstable; collect the missing IDs into a Vec (the existing missing variable),
sort it deterministically (e.g., sort() or sort_unstable()), and then assert on
the sorted missing vector so the "CHANGELOG.md is missing PR IDs: {:?}" output
is stable; update the logic around git_pr_ids, changelog_ids, and missing to
sort missing before the assert.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53464e30-e778-4977-8b43-b50ea6e6893f
📒 Files selected for processing (2)
CHANGELOG.mdtests/changelog_test.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/changelog_test.rs`:
- Around line 36-77: Replace the network-based logic inside
extract_github_merged_pr_ids() with a git-history-first approach: run "git log
--all --format=%s" (spawn a command instead of calling github_api_request) and
parse each subject line for PR number patterns (e.g., "Merge pull request `#123`",
"Merge branch '...'", and squash/merge suffixes like "… (`#123`)") to collect u32
PR IDs into the HashSet; only if this set is empty should you fall back to the
existing GitHub API loop (the code that constructs api_url and calls
github_api_request) as a last-resort path, and remove reliance on exported
GITHUB_TOKEN/other env vars for the primary path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a05e5312-7d58-48f9-991f-10530d45e7f7
📒 Files selected for processing (3)
.github/workflows/validate-agents.ymlCHANGELOG.mdtests/changelog_test.rs
Comment on lines
+36
to
+77
| fn extract_github_merged_pr_ids() -> Result<HashSet<u32>, String> { | ||
| let repo = env::var("GITHUB_REPOSITORY") | ||
| .unwrap_or_else(|_| "alchemiststudiosDOTai/sophon".to_string()); | ||
| let api_url = | ||
| env::var("GITHUB_API_URL").unwrap_or_else(|_| "https://api.github.com".to_string()); | ||
| let mut ids = HashSet::new(); | ||
|
|
||
| for page in 1.. { | ||
| let url = | ||
| format!("{api_url}/repos/{repo}/pulls?state=closed&base=main&per_page=100&page={page}"); | ||
| let output = github_api_request(&url)?; | ||
| let pulls: Vec<serde_json::Value> = serde_json::from_slice(&output) | ||
| .map_err(|error| format!("failed to parse GitHub API response: {error}"))?; | ||
|
|
||
| if pulls.is_empty() { | ||
| break; | ||
| } | ||
|
|
||
| for pull in &pulls { | ||
| if pull.get("merged_at").is_none_or(serde_json::Value::is_null) { | ||
| continue; | ||
| } | ||
| let number = pull | ||
| .get("number") | ||
| .and_then(serde_json::Value::as_u64) | ||
| .ok_or_else(|| "GitHub API response is missing a numeric PR number".to_string())?; | ||
| let id = u32::try_from(number) | ||
| .map_err(|error| format!("PR number {number} does not fit in u32: {error}"))?; | ||
| ids.insert(id); | ||
| } | ||
|
|
||
| if pulls.len() < 100 { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if ids.is_empty() { | ||
| return Err("GitHub API returned no merged PR IDs".to_string()); | ||
| } | ||
|
|
||
| Ok(ids) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current test is API-coupled and workflow exports token for it.
rg -n 'extract_github_merged_pr_ids|github_api_request|api.github.com|GITHUB_TOKEN|GH_TOKEN' tests/changelog_test.rs
rg -n 'Verify canonical command works|GITHUB_TOKEN' .github/workflows/validate-agents.yml
# After applying a git-first refactor, expected:
# - tests/changelog_test.rs no longer depends on api.github.com/github_api_request
# - workflow can drop the GITHUB_TOKEN env export for just checkRepository: alchemiststudiosDOTai/sophon
Length of output: 569
Replace GitHub API calls with git history parsing to eliminate flaky CI dependency
The current extract_github_merged_pr_ids() function (lines 36–77) relies on GitHub API calls, which can fail during rate-limit outages or API unavailability—blocking merges for infrastructure reasons unrelated to code quality. Additionally, this approach requires exporting GITHUB_TOKEN to the workflow's broader just check step.
Use git log --all --format=%s parsing instead, which captures both merge commits and squash commits via commit message patterns. Keep the GitHub API only as a true fallback if strictly required.
Proposed approach (git-first extraction)
- let merged_pr_ids = extract_github_merged_pr_ids().unwrap_or_else(|error| {
+ let merged_pr_ids = extract_git_merged_pr_ids().unwrap_or_else(|error| {
if is_ci() {
- panic!("failed to fetch merged PR IDs from GitHub: {error}");
+ panic!("failed to extract merged PR IDs from git history: {error}");
}
- eprintln!("skipping changelog merged-PR coverage outside CI: {error}");
+ eprintln!("skipping changelog merged-PR coverage outside CI: {error}");
HashSet::new()
});
@@
-fn extract_github_merged_pr_ids() -> Result<HashSet<u32>, String> {
- let repo = env::var("GITHUB_REPOSITORY")
- .unwrap_or_else(|_| "alchemiststudiosDOTai/sophon".to_string());
- let api_url =
- env::var("GITHUB_API_URL").unwrap_or_else(|_| "https://api.github.com".to_string());
- let mut ids = HashSet::new();
-
- for page in 1.. {
- let url =
- format!("{api_url}/repos/{repo}/pulls?state=closed&base=main&per_page=100&page={page}");
- let output = github_api_request(&url)?;
- let pulls: Vec<serde_json::Value> = serde_json::from_slice(&output)
- .map_err(|error| format!("failed to parse GitHub API response: {error}"))?;
-
- if pulls.is_empty() {
- break;
- }
-
- for pull in &pulls {
- if pull.get("merged_at").is_none_or(serde_json::Value::is_null) {
- continue;
- }
- let number = pull
- .get("number")
- .and_then(serde_json::Value::as_u64)
- .ok_or_else(|| "GitHub API response is missing a numeric PR number".to_string())?;
- let id = u32::try_from(number)
- .map_err(|error| format!("PR number {number} does not fit in u32: {error}"))?;
- ids.insert(id);
- }
-
- if pulls.len() < 100 {
- break;
- }
- }
-
- if ids.is_empty() {
- return Err("GitHub API returned no merged PR IDs".to_string());
- }
-
- Ok(ids)
+fn extract_git_merged_pr_ids() -> Result<HashSet<u32>, String> {
+ let output = Command::new("git")
+ .args(["log", "--all", "--format=%s"])
+ .output()
+ .map_err(|error| format!("failed to run git log: {error}"))?;
+
+ if !output.status.success() {
+ return Err(format!(
+ "git log failed with status {}: {}",
+ output.status,
+ String::from_utf8_lossy(&output.stderr)
+ ));
+ }
+
+ let mut ids = HashSet::new();
+ for subject in String::from_utf8_lossy(&output.stdout).lines() {
+ if let Some(id) = parse_merged_pr_id(subject) {
+ ids.insert(id);
+ }
+ }
+
+ if ids.is_empty() {
+ return Err("git history returned no merged PR IDs".to_string());
+ }
+ Ok(ids)
}
+
+fn parse_merged_pr_id(subject: &str) -> Option<u32> {
+ if let Some(rest) = subject.strip_prefix("Merge pull request #") {
+ let digits: String = rest.chars().take_while(|c| c.is_ascii_digit()).collect();
+ return digits.parse::<u32>().ok();
+ }
+ if let Some(start) = subject.rfind("(#") {
+ if subject.ends_with(')') {
+ let digits = &subject[start + 2..subject.len() - 1];
+ if !digits.is_empty() && digits.chars().all(|c| c.is_ascii_digit()) {
+ return digits.parse::<u32>().ok();
+ }
+ }
+ }
+ None
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/changelog_test.rs` around lines 36 - 77, Replace the network-based
logic inside extract_github_merged_pr_ids() with a git-history-first approach:
run "git log --all --format=%s" (spawn a command instead of calling
github_api_request) and parse each subject line for PR number patterns (e.g.,
"Merge pull request `#123`", "Merge branch '...'", and squash/merge suffixes like
"… (`#123`)") to collect u32 PR IDs into the HashSet; only if this set is empty
should you fall back to the existing GitHub API loop (the code that constructs
api_url and calls github_api_request) as a last-resort path, and remove reliance
on exported GITHUB_TOKEN/other env vars for the primary path.
larock22
pushed a commit
that referenced
this pull request
Jun 17, 2026
…ge-test feat: add changelog PR coverage test and backfill missing entries
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.
Summary
tests/changelog_test.rs— a test that asserts every merged PR number appears inCHANGELOG.md.How it works
#NPR references fromCHANGELOG.md#NPR references fromgit log --all(covers both merge and squash-merged PRs)Verification
cargo test --test changelog_testpassesjust checkpasses (all 66 tests, fmt, clippy, docs)Summary by CodeRabbit
Documentation
Tests
Bug Fixes
Chores