Add canonical shared-rules mechanism to claude-review-toolkit - #92
Add canonical shared-rules mechanism to claude-review-toolkit#92marcochavezf wants to merge 5 commits into
Conversation
The code-inline-reviewer agent enforces coding-standards rules from each repo's own committed copy, so a repo-agnostic rule like GEN-04 (commenting why) has already drifted into three different versions across Auth, Web-Expensify, and App. This copies genuinely shared rule files from shared-rules/ into the caller's rules directory at review time, so one canonical copy reaches every repo's real PR review without a per-repo commit to keep in sync.
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e21ca3e8a1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Issue #657167 exists specifically because the team decided to build a shared-rules mechanism instead of landing Auth PR #22682 (closed, never merged) directly, which would have added these sub-rules to Auth's copy only. Seeding the canonical GEN-04 file with only its original short-form content misses the point of doing this at all, so this carries over #22682's proposed content verbatim.
|
I have read the CLA Document and I hereby sign the CLA |
The unconditional mkdir -p created the caller's rules directory even when the caller never shipped one, so a repo missing .claude/skills/coding-standards/rules entirely would silently pass review with only the shared GEN-04 rule instead of failing the workflow as extractAllowedRules.sh's existing guard intends. Only copy shared rules into a directory the caller already has. Addresses: #92 (comment)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7250070914
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A directory-existence check alone still let a caller with an existing but empty or malformed rules directory pass, since the shared GEN-04 copy alone would satisfy extractAllowedRules.sh's check. Only copy shared rules in once the caller already has at least one rule file with a ruleId: tag matching extractAllowedRules.sh's own format, so a misconfigured caller still fails exactly as it does today. Addresses: #92 (comment)
Applies the house style (.claude/rules/plain-english-writing.md) and the GEN-04 sub-rules just canonicalized in shared-rules/ to the prose this PR itself added: split the dense multi-clause sentences, drop the redundant GEN-04-only parenthetical (the shared-rules directory listing already shows that), remove the one semicolon, and say "GH action" instead of the generic "the action" to match this repo's own terminology.
Details
code-inline-reviewerloads its coding-standards rules from each repo's own committed.claude/skills/coding-standards/rules/directory, so GEN-04 (commenting why) has already drifted into three versions. Auth's copy uses curly quotes, Web-Expensify's copy uses straight quotes under a different filename, and App has no GEN-04 at all.This adds
shared-rules/toclaude-review-toolkit, holding rules that are genuinely identical across repos. A new step inaction.ymlcopies each shared rule into the caller's rules directory asAutoLoadShared-<file>.md, before the existing allowlist-extraction step runs. That way, every caller's real PR review picks up the shared rule, with no per-repo copy to keep in sync.The canonical file also carries over six sub-rules from Auth's closed doc-only PR: no em dashes, no redundant parentheticals, no stacked hyphenated modifiers, no
->arrows, no semicolons, and no trailing comments. The tracking issue exists specifically to replace that PR, so seeding the canonical file with only today's short stub would have missed the point.Follow-up PRs in Auth, Web-Expensify, and App will bump their pinned
claude-review-toolkit@<sha>, delete their now-redundant local GEN-04 copy, and updatecode-inline-reviewer.md's docs-link generation once this merges.Related Issues
https://github.com/Expensify/Expensify/issues/657167
Manual Tests
Simulated the copy step locally against a test workspace with an existing local rule file, then ran
extractAllowedRules.shon the result and confirmed it extracts bothGEN-01(local) andGEN-04(shared) with no duplicates. Diffed the canonical file against Auth's livegen-04-commenting-why.mdand against the diff of Auth's closed doc-only PR to confirm both the base rule and the appended sub-rules are byte-identical to their sources.Linked PRs
None yet. Auth, Web-Expensify, and App consumer PRs will be linked here once opened, after this PR merges and its commit SHA is known.