Extend cargo-spellcheck to cover entire repository including .github directory #6062
Conversation
Signor1
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
WalkthroughThis pull request applies broad documentation and formatting refinements across the codebase, including standardizing inline code formatting for CIDs and other technical terms, correcting spelling and grammar, and updating configuration files. A functional change updates the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR involves numerous files, but the vast majority are repetitive documentation formatting changes (backticks around CIDs and code terms). The functional change in Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CONTRIBUTING.md (1)
106-110: Security contact is missingThe sentence ends with “sent by email to .” which omits the address/route. Link to SECURITY.md or provide the proper email.
-> Instead sensitive bugs must be sent by email to . +> Instead, sensitive bugs must be reported privately as described in our +> [SECURITY policy](./SECURITY.md).If you prefer an email, replace with your security contact address.
🧹 Nitpick comments (8)
documentation/src/developer_documentation/rpc_api_compatibility.md (1)
58-59: Minor wording tweak for clarityReplace “done with a local node rather than
api.node.glif.io” with “done against a local node rather than againstapi.node.glif.io.”-This means API compatibility testing has to be done with a local node rather than -`api.node.glif.io`. +This means API compatibility testing must be done against a local node rather than +against `api.node.glif.io`.documentation/src/developer_documentation/chain_index_spike.md (1)
86-88: Use canonical crate name and tighten phrasingPrefer “sqlx” (crate name) and simplify the sentence.
-2. **Sqlx** crate? In case of abstraction of the backend DB: +2. **sqlx** crate? If abstracting the backend DB:CHANGELOG.md (3)
90-93: Fix “computating” typoUse “computing.”
-- [#5946](https://github.com/ChainSafe/forest/pull/5946) Added `--n-epochs` to `forest-cli state compute` for computating state trees in batch. +- [#5946](https://github.com/ChainSafe/forest/pull/5946) Added `--n-epochs` to `forest-cli state compute` for computing state trees in batch.
1762-1763: Acronym casingPrefer “API” over “Api.”
-- `Build and Api Versioning` +- `Build and API Versioning`
1773-1774: Grammar nit“crashing when sending …” reads better.
-- Forest daemon crashing on sending bitswap requests. +- Forest daemon crashing when sending bitswap requests..config/forest.dic (3)
128-129: Normalize brand casing (optional)Consider dropping “CloudFlare” and keep “Cloudflare”.
-CloudFlare
271-273: Brand consistency (optional)Prefer “GitHub” over “Github/github” unless matching historical text.
-Github -github
1-680: Validate dictionary affix flags and remove duplicates
Dozens of entries in.config/forest.dicuse suffix flags (e.g.Algorand/M,CAR/SM,benchmark/GD); if your.afffile doesn’t define matching affix rules these will be ignored—either supply the needed affix definitions or strip the unused flags. Also scan for duplicate words via:awk '{print $1}' .config/forest.dic | sort | uniq -dand prune or consolidate any duplicates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.config/forest.dic(1 hunks).config/spellcheck.toml(1 hunks)CHANGELOG.md(9 hunks)CONTRIBUTING.md(1 hunks)documentation/src/developer_documentation/chain_index_spike.md(1 hunks)documentation/src/developer_documentation/rpc_api_compatibility.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T12:10:45.218Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5867
File: src/ipld/util.rs:553-558
Timestamp: 2025-08-08T12:10:45.218Z
Learning: Forest project targets Rust stable >=1.89; features stabilized in 1.88 like let-chains are acceptable in this codebase.
Applied to files:
CHANGELOG.md
🔇 Additional comments (4)
CONTRIBUTING.md (1)
115-117: LGTM: article fix“Open a bug report” reads correctly now.
CHANGELOG.md (2)
48-49: LGTM: added entry for extended spellcheckEntry accurately describes the change.
50-53: Mismatched issue number vs URLLink text shows #5897 but URL points to /issues/5987. Make them consistent.
-- [#5897](https://github.com/ChainSafe/forest/issues/5987) Added support for the NV27 _Golden Week_ network upgrade for devnets. -- [#5897](https://github.com/ChainSafe/forest/issues/5987) Added support for the NV27 _Golden Week_ network upgrade for calibration network. The upgrade epoch is set to `3_007_294` (Wed 10 Sep 23:00:00 UTC 2025). +- [#5987](https://github.com/ChainSafe/forest/issues/5987) Added support for the NV27 _Golden Week_ network upgrade for devnets. +- [#5987](https://github.com/ChainSafe/forest/issues/5987) Added support for the NV27 _Golden Week_ network upgrade for calibration network. The upgrade epoch is set to `3_007_294` (Wed 10 Sep 23:00:00 UTC 2025)..config/spellcheck.toml (1)
4-11: LGTM: sensible hunspell defaultsLanguage, dirs, and dictionaries look correct.
|
@Signor1 CI is failing can you check it. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
.config/forest.dic (7)
62-66: Prefer canonical forms; drop “Blockheader” and “Blocksync”.Keep “BlockHeader”, “BlockSync”, and “blocksync”.
- Blockheader - Blocksync
27-27: Brand style: “Arch Linux” is two words.Recommend removing “Archlinux” and fixing any prose to “Arch Linux”.
- Archlinux
368-369: Use brand-correct “mdBook”.Add “mdBook” (tool name). Keep “mdbook” if it appears as a CLI/crate; you can drop “MdBook” if not used.
+ mdBook - MdBook
191-191: Prefer “e.g.” over “eg”; keep “i.e.”.Add the standard abbreviation and remove the non‑standard form.
- eg + e.g.Also applies to: 293-293
340-340: Remove “LetsEncrypt”.Use “Let’s Encrypt” (with apostrophe and space) in prose; no need to whitelist “LetsEncrypt”.
- LetsEncrypt
341-344: libp2p brand-case: keep only “libp2p” unless mixed-case is intentional.If not required by historical text, drop mixed‑case variants to avoid masking style drift.
- LibP2P - Libp2p
151-153: Add hyphenated standards names.Docs often use “DAG‑JSON” and “DAG‑CBOR”; include these alongside existing variants.
+ DAG-JSON + DAG-CBOR
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.config/forest.dic(1 hunks)
🔇 Additional comments (4)
.config/forest.dic (4)
2-676: Strong expansion; good coverage.The broadened allowlist looks comprehensive and aligns with the PR goal to catch repo‑wide typos, including .github. No structural issues spotted with the file format.
447-448: Style choice: en‑US vs en‑GB (“organisation/Organisation”).Confirm the house style. If en‑US, remove these and use “organization/Organization” in prose; if mixed allowed, keep as is.
557-557: Remove “Stackoverflow”.Brand is “Stack Overflow” (two words); don’t whitelist the typo.
- StackoverflowLikely an incorrect or invalid review comment.
2-676: Fix typos, trailing punctuation, and canonicalize brand casing in .config/forest.dic
- Remove trailing-period entries:
backend.,bitswap.- Correct obvious misspellings: line 206
everyting→everything; line 449overriden→overridden- Fix abbreviation: line 190
eg→e.g.(or remove if not desired)- Canonicalize/dedupe brand entries (pick one preferred casing and remove other variants):
- github: ['Github', 'github']
- cloudflare: ['Cloudflare', 'CloudFlare']
- libp2p: ['libp2p', 'LibP2P', 'LibP2p']
- bitswap: ['bitswap', 'Bitswap']
- blocksync: ['BlockSync', 'Blocksync', 'blocksync']
- blockheader: ['BlockHeader', 'Blockheader']
- Other flagged tokens (for review/fix): 27:Archlinux, 62:Blockheader, 64:Blocksync, 127:CloudFlare, 269:Github, 339:LetsEncrypt, 556:Stackoverflow
- Re-run spellcheck:
make spellcheckorcargo spellcheck check --cfg .config/spellcheck.toml --recursive
.config/forest.dic
Outdated
| Ethereum | ||
| eth | ||
| exa | ||
| everyting |
There was a problem hiding this comment.
Delete obvious typos (“everyting”, “overriden”).
These undermine the value of spellcheck.
- everyting
- overridenAlso applies to: 450-450
🤖 Prompt for AI Agents
In .config/forest.dic around line 206 (and also at line 450), there's an obvious
typo "everyting" (and similarly "overriden" per reviewer); update the dictionary
entries to correct spellings ("everything" and "overridden") so the spellchecker
file contains valid words, preserving file formatting and any surrounding
entries.
|
Hi @akaladarshi, I'm having challenges installing cargo-spellcheck locally. error: failed to run custom build command for `hunspell-sys v0.3.1`So I had to use CI to check. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.config/forest.dic (3)
42-42: Remove tokens with trailing punctuation (they mask punctuation errors).These entries will hide sentence-final punctuation mistakes.
- backend. - bitswap.Also applies to: 58-58
449-449: Delete typo “overriden” to avoid whitelisting a common mistake.- overriden + overridden
556-556: Use canonical brand spelling “Stack Overflow”; drop “Stackoverflow”.- StackoverflowPrefer “Stack Overflow” in prose.
🧹 Nitpick comments (3)
.config/forest.dic (3)
358-359: Prefer canonical casing “macOS”; remove “MacOS”.- MacOS
175-175: Standardize “DevOps” casing (optionally include lowercase variant for prose).- Devops + DevOps + devops
189-189: Prefer “e.g.” over “eg”.Aligns with “i.e.” already present and avoids normalizing a style issue.
- eg + e.g.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.config/forest.dic(1 hunks)
🔇 Additional comments (1)
.config/forest.dic (1)
1-676: Overall: solid expansion; good coverage and useful normalizationsThe bulk of additions look appropriate for reducing false positives across the repo. Nice cleanup of prior typos and inclusion of common project terms.
|
Hey @Signor1 could you run |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
.config/forest.dic (4)
41-59: Remove tokens with trailing punctuation (will mask sentence-end typos).Keep only the bare tokens; drop the dotted forms.
- backend. - bitswap.
139-141: Remove stray non-word “creativ” (keep “creativcoder”).- creativ
438-438: Don’t whitelist placeholder tokens; drop “nvXX”.- nvXX
450-450: Delete obvious typo “overriden” (use “overridden” in prose).- overriden
🧹 Nitpick comments (7)
.config/forest.dic (7)
54-56: Normalize BitField spelling; drop mixed-case “Bitfield”.Prefer
BitField(type) andbitfield(common noun); removeBitfield.- Bitfield
62-65: Normalize BlockHeader/BlockSync casing; remove mid-word lowercase variants.Use
BlockHeaderandBlockSync; dropBlockheaderandBlocksync.- Blockheader - Blocksync
27-27: Use canonical brand “Arch Linux”; drop “Archlinux”.Enforce correct casing/spacing in docs; the dictionary entry isn’t needed.
- Archlinux
359-360: Prefer “macOS”; drop “MacOS”.Keep only the canonical Apple branding.
- MacOS
557-557: Prefer “Stack Overflow”; drop “Stackoverflow”.Use the official brand style in docs.
- Stackoverflow
340-340: Prefer “Let’s Encrypt”; don’t whitelist “LetsEncrypt”.Avoid encouraging the non-canonical form.
- LetsEncrypt
21-26: Consider dropping mixed-case “Api”; prefer “API” (acro) or “api” (identifier).Reduces inconsistent styling in docs.
- Api
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.config/forest.dic(1 hunks)docs/README.md(1 hunks)f3-sidecar/README.md(1 hunks)src/shim/actors/builtin/miner/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/shim/actors/builtin/miner/mod.rs
- docs/README.md
- f3-sidecar/README.md
|
Hey @hanabi1224, |
Also, you can let me know if the spellcheck implementation inside the docs folder should be discarded. |
|
Hey @hanabi1224 , there's an unrelated CI failure in the docs deployment workflow where yarn dependencies |
@Signor1 All commits have to to signed to unblock merge. Could you sign all of them?
That's fine, we can keep it.
Retrying, if could still fail due to insufficient permission to deploy from a fork but never mind. |
|
@hanabi1224 , Alright. Meanwhile, I just signed this PR #6062 |
|
Hey @Signor1 it requires all commits to be signed before we can merge this PR, please check and make sure every commit has a checkmark https://github.com/ChainSafe/forest/pull/6062/commits |
Signor1
dismissed stale reviews from akaladarshi and hanabi1224
via
d504946
Signor1
force-pushed
the
fix-spellcheck
branch
from
1220933 to
d504946
Compare
|
@elmattic, I've resolved the typos. |
|
@Signor1 Please make sure the CI check is passing |
|
@akaladarshi CI check is fixed. Can this PR be duly checked and merged ? |
|
@Signor1 CI is still failing you can run the check locally once and verify |
|
@akaladarshi |
| "bafk2bzacedgl6zt7d2ointmeyqrdwkehpi5w5rk3smsemrwbf3v5gc2wx7qfy" | ||
| ] | ||
| ], | ||
| "actor_list_cid": "bafy2bzaceasz5pov3d4tyxxdcxj2qvvdnkdxihzycj5oos5t2xj5pznwvxvem" |
There was a problem hiding this comment.
@Signor1 Can you please revert this change
There was a problem hiding this comment.
This might be the reason the unit test is failing.
There was a problem hiding this comment.
Hey @akaladarshi, this version is the same with what is in base branch.
There was a problem hiding this comment.
@Signor1 The unit test state_migration_generate_actors_metadata is failing because of the changes, it might be related to the manifest file.
Can you please verify once by running make test or cargo nextest run --workspace --no-fail-fast.
There was a problem hiding this comment.
@akaladarshi I've identified the fix :)
I added a trailing new line to the manifest.json. Absence of \n at the end of manifest.json made this comparison assert_eq!(json, manifest)to fail
There was a problem hiding this comment.
@akaladarshi I've identified the fix :)
I added a trailing new line to the
manifest.json. Absence of\nat the end of manifest.json made this comparisonassert_eq!(json, manifest)to fail
Since it is a byte-for-byte string comparison, the \n counts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.config/spellcheck.toml (1)
38-40: Empty config sections introduced without documentation or purpose.The new
[nlprules]and[reflow]sections are added but appear empty or minimally configured. Clarify whether these sections are:
- Intentional placeholders for future use (document this intention)
- Incomplete configuration that should have values (complete them)
- Dead code that should be removed
Empty sections may signal incomplete work or configuration drift. If retained, add inline comments explaining their purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.config/spellcheck.toml(1 hunks)config.toml(1 hunks)
🔇 Additional comments (2)
config.toml (1)
1-46: Verify scope alignment with PR objectives.This config file does not appear related to the stated PR objectives (extending cargo-spellcheck coverage and dictionary updates). Please confirm:
- Is this file intentionally part of the spellcheck PR, or was it accidentally included?
- Is this file meant as a default configuration, example template, or documentation?
- Should it be included in version control at all, or should it be .gitignore'd (e.g., if it's meant as a user-specific template)?
Based on PR objectives and typical practices, config files with user-specific paths are usually not committed to repositories directly.
.config/spellcheck.toml (1)
9-9: Configuration changes align well with Markdown spellcheck objectives.The tokenization updates (line 9) and new transform_regex patterns (lines 16–32) appropriately handle Markdown-specific constructs:
- Template variables (
{{ ... }}) and Handlebars/Jinja syntax- Double braces for standalone delimiters
- Numeric and operator sequences
- Emoji support (with accompanying
allow_emojis = trueon line 36)- Import-like patterns (
@theme/...)These changes support the PR's goal of extending spellcheck coverage to
.githuband other Markdown files without excessive false positives on code-like tokens.Also applies to: 16-36
|
@akaladarshi please review. I've fixed the issue |
There was a problem hiding this comment.
The dictionary should not contain entries such as class or module names, different casing of wasm, implementer/implementor (just choose one) etc. Some features of hunspell could be used to limit the number of entries (refer to https://github.com/ChainSafe/forest/blob/main/.config/spellcheck.md). Please ensure all entries in the dictionary are correct and needed (refer to https://github.com/ChainSafe/forest/blob/main/.config/spellcheck.md#tips for stuff like NV22).
Also, if cargos-spellcheck is used across the board, the javascript one at docs/ should be removed - there's no point in having two. Refer to @hanabi1224 comment in your previus PR #6058 (comment).
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/daemon/bundle.rs (1)
124-126: Fix type mismatch in comparison.Line 125 compares
Option<&Cid>(returned byfirst()) with&Ciddirectly, which would cause a compilation error sincePartialEqis not implemented between these types. Additionally, the error messages could be more descriptive for debugging.Apply this diff to fix the type mismatch and improve error messages:
let header = stream.header_v1; - anyhow::ensure!(header.roots.len() == 1); - anyhow::ensure!(header.roots.first() == root); + anyhow::ensure!( + header.roots.len() == 1, + "Expected exactly one root in bundle {network}-{version}, found {}", + header.roots.len() + ); + anyhow::ensure!( + header.roots.first() == Some(root), + "Bundle {network}-{version} root mismatch: expected {root}, found {:?}", + header.roots.first() + ); Ok(*root)
🧹 Nitpick comments (1)
src/daemon/bundle.rs (1)
91-123: LGTM with minor suggestions!The concurrent processing with
FuturesUnordered, filtering logic, download fallback mechanism, and streaming into the database are all well-implemented.Minor suggestions:
- Line 95: The
unwrap_or(false)fallback is reasonable but could mask underlying database errors. Consider logging a warning ifdb.has()returns an error.- Line 114: Good error handling with the warning message for fallback URL.
Consider adding a warning log on line 95 if the database check fails:
.filter(|bundle| { - !db.has(&bundle.manifest).unwrap_or(false) && + !db.has(&bundle.manifest).unwrap_or_else(|e| { + warn!("Failed to check if bundle exists: {e}"); + false + }) && // Comparing only the discriminant is enough. All devnets share the same // actor bundle. discriminant(network) == discriminant(&bundle.network)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(16 hunks)src/chain/mod.rs(2 hunks)src/daemon/bundle.rs(1 hunks)src/tool/subcommands/archive_cmd.rs(1 hunks)src/utils/db/car_stream.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/db/car_stream.rs
- CHANGELOG.md
🔇 Additional comments (4)
src/tool/subcommands/archive_cmd.rs (1)
153-155: LGTM! Documentation formatting improved.The addition of backticks around version identifiers (
v1andv2) correctly formats these technical terms as inline code, improving documentation clarity and consistency.src/chain/mod.rs (1)
52-52: LGTM! Documentation formatting improvement.Wrapping version identifiers in backticks follows rustdoc best practices and aligns with reviewer feedback about preferring inline code formatting over adding technical terms to the spellcheck dictionary.
Also applies to: 65-65
src/daemon/bundle.rs (2)
85-85: LGTM!The documentation correctly describes the return value, and the backtick formatting around
CIDsis appropriate.
86-90: The unused return value is intentional and acceptable.The function documentation explicitly states it returns "the
CIDsof the loaded bundles." Both call sites discard this value, which is acceptable—this design allows future callers to access the CIDs when needed while current callers that don't require them can simply ignore the return value.
5 participants
Summary of changes
Changes introduced in this pull request:
The motivation was to expand spellcheck coverage from only
docs/docsdirectory to the entire repository, ensuring consistent spelling standards across all documentation including GitHub workflows, README files, and other markdown content.Reference issue to close (if applicable)
Closes #[5958]
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Documentation
Chores