chore: sort snapshot lists#6033
Conversation
LesnyRumcajs
requested review from
akaladarshi and
elmattic
and removed request for
a team
WalkthroughAdds a CI job that sorts and verifies snapshot list files, a Makefile target to lexicographically sort/deduplicate those files, and updates two snapshot list files (one with extensive content changes and reordering, one with a small reordering). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor GH as GitHub Actions
participant Runner as ubuntu-24.04-arm
participant Repo as Repository
participant Make as make (sort-lists)
participant Git as git (diff --exit-code)
GH->>Runner: start lists-lint job
Runner->>Repo: actions/checkout@v4
Runner->>Make: run make sort-lists
Note right of Make #DFF2E1: for each file in LIST_FILES\nrun `sort --unique $@ -o $@`
Make-->>Runner: exit status
Runner->>Git: git diff --exit-code
alt no changes
Git-->>Runner: exit 0
Runner-->>GH: job success
else changes detected
Git-->>Runner: exit 1
Runner-->>GH: job failure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🪛 GitHub Check: CodeQL.github/workflows/scripts-lint.yml[warning] 85-91: Workflow does not contain permissions ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (6)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
Makefile (1)
130-130: Don’t mark real files as phony targets.Declaring the list files as .PHONY defeats make’s incremental logic and can cause unnecessary rewrites.
-.PHONY: sort-lists $(LIST_FILES) +.PHONY: sort-listssrc/tool/subcommands/api_cmd/test_snapshots.txt (1)
1-289: Consider regenerating this file via the new make target to avoid manual drift.Given multiple ordering slips, prefer “make sort-lists” (now enforced in CI) to keep this fixture canonical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/scripts-lint.yml(1 hunks)Makefile(1 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(4 hunks)src/tool/subcommands/api_cmd/test_snapshots_ignored.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
🪛 actionlint (1.7.7)
.github/workflows/scripts-lint.yml
89-89: could not parse as YAML: yaml: line 89: could not find expected ':'
(syntax-check)
🪛 GitHub Check: CodeQL
.github/workflows/scripts-lint.yml
[warning] 85-85: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 YAMLlint (1.37.1)
.github/workflows/scripts-lint.yml
[error] 90-90: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Go lint checks
- GitHub Check: cargo-publish-dry-run
- GitHub Check: All lint checks
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
src/tool/subcommands/api_cmd/test_snapshots_ignored.txt (1)
22-23: LGTM: ordering correction for EthSyncing.The move places EthSyncing in proper lexicographic order between EthSubscribe and EthUnsubscribe.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/scripts-lint.yml (1)
85-93: Harden GITHUB_TOKEN permissions for this job.Add minimal permissions to satisfy CodeQL and least-privilege.
lists-lint: + permissions: + contents: read runs-on: ubuntu-24.04-arm
🧹 Nitpick comments (1)
.github/workflows/scripts-lint.yml (1)
84-94: Align merge-queue behavior (optional).python-lint skips gh-readonly-queue; consider mirroring for consistency, unless you explicitly want this job in the queue.
- lists-lint: + lists-lint: + if: ${{ !startsWith(github.ref, 'refs/heads/gh-readonly-queue/') }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/scripts-lint.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
🪛 GitHub Check: CodeQL
.github/workflows/scripts-lint.yml
[warning] 85-93: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: tests-release
- GitHub Check: Build MacOS
- GitHub Check: tests
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: All lint checks
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
.github/workflows/scripts-lint.yml (1)
84-94: Nice addition: deterministic sort check with locale pinned.The lists-lint job and LC_ALL=C ensure stable, byte-wise ordering; the diff gate will catch regressions. LGTM.
4 participants
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6001 (comment)
Other information and links
Change checklist
Summary by CodeRabbit
Chores
Tests
Impact