feat: add /format skill for pre-commit formatting checks#275
Conversation
Add a new Claude skill that runs all formatting and cleaning tasks: - Rust formatting (cargo +nightly fmt) - TOML formatting (taplo) - Zepter feature propagation checks - Clippy linting Update CLAUDE.md to instruct Claude to automatically run /format after generating code and before creating commits.
There was a problem hiding this comment.
I'm a bit scepitcal when it comes to any skills
Isn't this general rust knowledge of Claude to figure out these command?
Aren't commands already existing in CLAUDE.md sufficient?
There was a problem hiding this comment.
I can understand your scepticism but this way we're explicitly stating what we expect from Claude.
Things like linting and formatting rarely change so it won't be a maintenance burden
There was a problem hiding this comment.
I think CLAUDE.md is enough, I just tell format and it does all "rustfmt, taplo, zepter"
There was a problem hiding this comment.
Specifying explicitly wherever possible, always helps. Cant bank on Claude to assume, so explicit is better here.
| **Automatic formatting:** | ||
| - ALWAYS run `/format` after generating or modifying Rust code | ||
| - ALWAYS run `/format` before creating any git commit | ||
| - This ensures all code follows project formatting standards (Rust, TOML, feature propagation) and passes clippy |
There was a problem hiding this comment.
Wouldn't these be enough without the SKILL.md ?
There was a problem hiding this comment.
/format will be both undeclared and undefined then and Claude won't know what to do
There was a problem hiding this comment.
Anthropic merged commands and skills. So by having this skill you can tell claude to /format and it'll do it or you can tell it to format and it would figure out to use this skill
There was a problem hiding this comment.
yes, but I didn't mean to leave /format while removing the SKILL.md - I mean change to something like
- ALWAYS run format after generating or modifying Rust code
There was a problem hiding this comment.
all these commands we also have mentioned inside CLAUDE.md, does separate skill bring any value ?
There was a problem hiding this comment.
and what about bloating the context window with unnecessary stuff ? that's my biggest concern with all those skills that are introducing knowledge which is already generally available
There was a problem hiding this comment.
Wouldn't these be enough without the SKILL.md ?
No it wont. If we just put it in this file and not as a skill, we are then forcing people to use claude for commit, pull, push everything. We should provide an option to the user to use claude for what they want to. With skills we will give an option for people to use /format in claude to format everything and then if they can use git commands on their own.
Adding in this file (Claude.md) for those who want to use claude even for git commands.
There was a problem hiding this comment.
Wouldn't these be enough without the SKILL.md ?
No it wont. If we just put it in this file and not as a skill, we are then forcing people to use claude for commit, pull, push everything. We should provide an option to the user to use claude for what they want to. With skills we will give an option for people to use /format in claude to format everything and then if they can use git commands on their own.
Adding in this file (Claude.md) for those who want to use claude even for git commands.
Sorry @mudigal but I dont understand. Could rephrase maybe ?
Nothing in CLAUDE.md can force me to use Claude instead of git command if I wish to.
Maybe I explained it poorly but I'm arguing that having exactly the same commands in both files Claude.md and Skill.md - doesn't bring any new value to Claude. Claude by being aware of Claude.md already knows how to format and lint and all - bringing another file to Claude looks like bloating the context for no gain
There was a problem hiding this comment.
The key distinction here is how different configuration files behave:
Claude.md — Instructions here only apply when you explicitly run Claude (e.g., "commit and push"). If we document "check for formatting before committing" in Claude.md, it will only trigger when you use Claude for the commit. This is automatic behavior that Claude follows whenever it's invoked.
Skills.md — This defines explicit skills that users can invoke on demand, like /format. When a user runs /format through Claude, it performs only that specific task (formatting) and nothing else.
This approach gives users flexibility:
- They can let Claude handle everything automatically via Claude.md instructions, or
- They can explicitly invoke specific skills (like /format) when they only need that particular action
The user stays in control of what they want Claude to do. Having this choice is valuable for different workflows and preferences.
There was a problem hiding this comment.
and what about bloating the context window with unnecessary stuff ?
Let's be honest, the context won't blow if you add a few extra lines to it. Claude is smart enough to strip or compact the context whenever needed.
If it comes to skills, only their short description is loaded into Claude's context. The full description is lazily uploaded into the context memory whenever needed, so I would not worry about skills overflowing the context window either, especially with modern context windows of 1M+ tokens
franciscoaguirre
left a comment
franciscoaguirre
left a comment
There was a problem hiding this comment.
I agree with the skill, but I would also create a hook to format on push
| - NEVER add Co-Authored-By lines to commits | ||
| - NEVER use git push --force or git push -f | ||
|
|
||
| **Automatic formatting:** |
There was a problem hiding this comment.
This should probably be moved to a hook
There was a problem hiding this comment.
I see the value in using a git hook for automatic enforcement. However, one consideration is that adding formatting to a pre-push hook would add time to every push — the hook would run formatting on all files before allowing the push to complete. For our codebases, this could add noticeable delay (30+ seconds) to each push.
IMO, the Skills.md approach provides more flexibility.
Co-authored-by: Rohit Sarpotdar <rohit.sarpotdar@parity.io>
Remove unnecessary `&` references on `scope` parameter in check_authorization and check_authorization_expired functions. The parameter is already a reference, so `&scope` creates a double-reference which clippy flags as needless. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* feat: add /format skill for pre-commit formatting checks Add a new Claude skill that runs all formatting and cleaning tasks: - Rust formatting (cargo +nightly fmt) - TOML formatting (taplo) - Zepter feature propagation checks - Clippy linting Update CLAUDE.md to instruct Claude to automatically run /format after generating code and before creating commits. * Update SKILL.md description to include linting * Update .claude/skills/format/SKILL.md Co-authored-by: Rohit Sarpotdar <rohit.sarpotdar@parity.io> --------- Co-authored-by: Andrii <ndk@parity.io> Co-authored-by: Rohit Sarpotdar <rohit.sarpotdar@parity.io>
* Improvements: simply authorizations, missing xcm configs, test coverage * Adapt benchmarking * Remove call level auhtorization consumption * improvements and tests * Pass authorization via custom origin for signed transactions * add test for prepare * Apply suggestions from code review Co-authored-by: Branislav Kontur <bkontur@gmail.com> * Update pallets/transaction-storage/src/lib.rs Co-authored-by: Branislav Kontur <bkontur@gmail.com> * replacing ipfs-http-client usage (#272) * replacing ipfs-http-client usage * remove unused const * added ValidTransaction tag for check_signed (#269) * added ValidTransaction tag for check_signed * Apply suggestion from @bkontur * Fix call to renamed parameter in check_store_renew_unsigned The `hash` parameter was renamed to `content_hash` but the call site was not updated, causing a compilation error. * Unify preimage ValidTransaction tag between signed and unsigned paths Extract preimage_store_renew_valid_transaction helper so both check_store_renew_unsigned and check_signed use the same tag prefix and provides, ensuring the tx pool deduplicates across signed and unsigned submissions of the same pre-authorized content. * Test that signed preimage tags match unsigned preimage tags Extend validate_signed_account_authorization_has_provides_tag to verify that the preimage-authorized signed path produces the same ValidTransaction provides tag as the unsigned path, ensuring the transaction pool deduplicates across both submission types. * Test cross-signer dedup for preimage-authorized content Verify that different signers submitting the same preimage-authorized content produce identical provides tags, confirming dedup is content-based and not signer-based. --------- Co-authored-by: Branislav Kontur <bkontur@gmail.com> * Minor cleanups: pass scope by ref, remove unused deps, add Dependabot npm grouping (#276) * Pass scope by reference in check_authorization and check_authorization_expired * Removed unused * ci: add Dependabot npm monitoring for console-ui and examples Group version and security updates per directory so each produces a single PR instead of one per dependency. * fmt :D * Revert and fix CI * feat: add /format skill for pre-commit formatting checks (#275) * feat: add /format skill for pre-commit formatting checks Add a new Claude skill that runs all formatting and cleaning tasks: - Rust formatting (cargo +nightly fmt) - TOML formatting (taplo) - Zepter feature propagation checks - Clippy linting Update CLAUDE.md to instruct Claude to automatically run /format after generating code and before creating commits. * Update SKILL.md description to include linting * Update .claude/skills/format/SKILL.md Co-authored-by: Rohit Sarpotdar <rohit.sarpotdar@parity.io> --------- Co-authored-by: Andrii <ndk@parity.io> Co-authored-by: Rohit Sarpotdar <rohit.sarpotdar@parity.io> * Review fixes * Improvements * Nit * Add comment explaining ValidTransaction construction logic * Replace EnsureAuthorized with fn ensure_authorized Replace the EnsureAuthorized struct (EnsureOrigin impl) with a Pallet::ensure_authorized() method that validates store/renew origins and returns a rich AuthorizedCaller enum. The extrinsics now explicitly check their origin instead of ignoring it. - Add AuthorizedCaller enum with Signed/Root/Unsigned variants - Add ensure_authorized() in impl Pallet<T> using into_caller().try_into() to extract the pallet Origin::Authorized, with fallbacks to root/none - Add TryInto<Origin<Self>> bound to Config via frame_system supertrait - Update store, store_with_cid_config, renew to call ensure_authorized - Remove EnsureAuthorized struct and its EnsureOrigin impl - Update tests to use ensure_authorized and AuthorizedCaller * Rename * Rename ValidateSigned -> AllowedSignedCalls, AuthorizeStorageSigned -> ValidateStorageCalls Rename transaction extensions to better reflect their purpose: - ValidateSigned -> AllowedSignedCalls: acts as a signed-call allowlist - AuthorizeStorageSigned -> ValidateStorageCalls: validates TransactionStorage calls Add test verifying AllowedSignedCalls preserves priority set by ValidateStorageCalls for TransactionStorage calls. * Add benchmarked weights for ValidateStorageCalls extension Add validate_store() and validate_renew() to WeightInfo trait and benchmark them using test_run() (validate + prepare + post_dispatch without executing the extrinsic). Wire extension's weight() method to dispatch to the benchmarked weights instead of hardcoded DbWeight estimates. Add placeholder weights to both runtime weight files. * Fix fmt for CI nightly and update ValidateStorageCalls doc comment * Expand ValidateStorageCalls doc: authorization is consumed in prepare * Fresh weights * Apply Karol's comment * Simplify ValidateStorageCalls: pass only signer via Val, not scope The scope is only needed in validate() to transform the origin and is unused in prepare(). Simplify Val from Option<(AccountId, Scope)> to Option<AccountId> and unify the two prepare() branches into a single val.or(origin) lookup. * Parameterize validate_store weight by data length The validate_store weight was ~18s because the benchmark used MaxTransactionSize (8MB). The expensive operation is blake2_256(data) which scales linearly with data size. Parameterize the benchmark and weight function by data length so small transactions get proportionally smaller weights. * Fresh weights from 195.154.91.224 * Move CallInspector to extension file * Extract common code, remove dead code * Clean code improvements * Formatting/clippy * mempool deduplication * Transform origin only for store/renew * Code review, tests, deduplication * Add logs related to traversal * Add more tests and sync existing ones between solochain and parachain * Clippy fix * Apply suggestions from code review Co-authored-by: Branislav Kontur <bkontur@gmail.com> * Adapt to review changes * Move is_storage_mutating_call into CallInspector trait as a provided method (#327) Refactor the standalone `is_storage_mutating_call` function into a provided method on the `CallInspector` trait. This changes the trait parameter from `CallInspector<Call>` to `CallInspector<T: Config>`, enabling the cleaner `Self::is_storage_mutating_call(call, 0)` syntax in runtime `Contains` impls instead of the verbose turbofish `pallet_transaction_storage::is_storage_mutating_call::<Runtime, Self>(call, 0)`. * Move traverse_storage_calls to the CallInspector * Fix clippy * Missing variables in tests genesis set-up * Reject wrapped store/renew, keep management call validation (#332) * Reject wrapped store/renew, keep management call validation Storage-mutating calls (store, store_with_cid_config, renew) must now be submitted as direct extrinsics only — wrapping them in batch, sudo, proxy, etc. is rejected at validation time with InvalidTransaction::Call. Authorization management calls (authorize_*, refresh_*, remove_expired_*) can still be wrapped and are validated via validate_signed as before. This simplifies the extension logic by removing: - preserves_origin tracking from TraverseResult and inspect_wrapper - Mixed-batch rejection logic (store + management in same batch) - Origin transformation for wrappers (no longer needed) - authorized_scope accumulation in the wrapper path The visitor now skips validate_signed for storage-mutating calls found in wrappers (they'll be rejected by the has_storage_mutating check), ensuring the correct InvalidTransaction::Call error is returned regardless of whether the caller has authorization. * simplify * More tests * Avoid unnecessary clones in extract_signer and ValidateStorageCalls (#337) * Avoid cloning RuntimeOrigin in extract_signer Use origin.caller() to match on OriginCaller by reference instead of cloning the entire RuntimeOrigin via into_caller(). Only the AccountId is cloned, which is needed for the return value. * Avoid unnecessary clone of scope in ValidateStorageCalls Move scope into Authorized origin instead of cloning it, since maybe_scope is owned and not used after this point. * Allow sudo(store) on bulletin-westend Remove Sudo from StorageCallInspector on Westend so the sudo key holder can store data via sudo(store) without authorization. Root origin is already accepted by ensure_authorized. Update existing sudo_as tests to reflect that sudo calls now pass validation (failing at dispatch instead when no sudo key is configured). * nit * nit * Remove stale test note about Westend call allowlist * Code-review improvements * Simplify ValidateStorageCalls::prepare by removing redundant branching (#338) traverse_storage_calls already handles both direct pallet calls (via is_sub_type) and wrapper calls (via inspect_wrapper), so the separate code paths in prepare() were unnecessary. --------- Co-authored-by: RafalMirowski1 <rafal@parity.io> Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: Rohit Sarpotdar <rohit.sarpotdar@parity.io> Co-authored-by: Naren Mudigal <mudigal@users.noreply.github.com> Co-authored-by: Andrii <ndk@parity.io>
6 participants
Summary
/formatClaude skill that runs all formatting and cleaning tasks (Rust fmt, TOML formatting, zepter, clippy)/formatafter generating code and before commitsTest plan
/formatskill is available in Claude Code/formatexecutes all formatting commands