| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
📝 Walkthrough
WalkthroughSubjectImperativeValidator now detects non-imperative morphology instead of requiring vocabulary membership. It permits imperative words, documented lookalikes, adverb-led subjects, noun-led subjects, and existing merge or fixup bypasses. ChangesImperative morphology validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SubjectImperativeValidator
participant NON_IMPERATIVE_LOOKALIKES
participant IMPERATIVES
SubjectImperativeValidator->>NON_IMPERATIVE_LOOKALIKES: Check first-word exception
SubjectImperativeValidator->>IMPERATIVES: Check third-person s stem
SubjectImperativeValidator->>SubjectImperativeValidator: Return pass or fail
Possibly related PRs
Suggested labels: tests 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
Commit Check✅ All 10 checks passed Show all 10 checksCommit message
✔ PR title (feat: check imperative mood by form rather than by vocabu...)
✔ Commit 1/6 (feat: check imperative mood by form rather than by vocabu...)
✔ Commit 2/6 (fix: keep noun-led subjects that end in a single s)
✔ Commit 3/6 (fix: quote the codespell ignore list so the comma survives)
✔ Commit 4/6 (test: check the -ies stem, which nothing exercised)
✔ Commit 5/6 (refactor: stop asking the verb list whether a subject passes)
✔ Commit 6/6 (fix: complete the two word families the morphology depend...)
Branch
✔ Branch (claude/imperative-morphology-602anc)
Author
✔ Author name (Xianpeng Shen)
✔ Author email (xianpeng.shen@gmail.com)
commit-check 2.13.4 · Rules reference |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@commit_check/engine.py`: - Around line 452-465: Update commit_check/engine.py lines 452-465 in _is_inflected to distinguish third-person verb forms from noun-led words ending in a single “s”, preserving valid subjects such as “status report” while retaining detection of actual inflected verbs. In commit_check/imperatives.py lines 563-596, if the implementation relies on exceptions, add regression-covered non-inflected single-“s” examples and document the maintenance boundary. In `@tests/engine_test.py`: - Around line 2603-2608: Update the test subjects in the listed cases to use bare infinitive verbs that are absent from IMPERATIVES, replacing settle, inline, retire, tighten, and treat while preserving the existing report-format, helper, legacy-path, guard, and absent-value contexts. Ensure the cases exercise the morphology fallback rather than the fast path.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aba3f9c5-171c-45e7-8243-a9985fe1244e
📥 CommitsReviewing files that changed from the base of the PR and between 61bd994 and c6fbd53.
📒 Files selected for processing (3)
Sorry, something went wrong.
CC003 asked whether the first word appeared in a list of imperative verbs. Mood is a property of a word's form, so the list rejected correct subjects wherever it fell short -- and it always fell short. On 59k strictly imperative subjects from git.git it rejected 45%. Ask the opposite question instead: reject a first word that carries non-imperative morphology (a past tense, a gerund, a third person) and accept everything else. That rejects 1% of the same corpus. Closes #526
A trailing -s is weaker evidence than -ed or -ing: plural nouns wear
one too, so 'fix: status report' was rejected. Require corroboration
for that suffix only -- the stem has to be a verb already known -- which
drops 1175 false positives on the git.git corpus and leaves the
genuinely ambiguous cases ('notes', 'tests') read as verbs.
Also trims the comments this PR added, drops the lookalike entries the
change makes unnecessary, and stops codespell tripping on 'sting'.
In a YAML flow sequence an unquoted comma is an item separator, so [--ignore-words-list=iterm,sting] passed codespell two arguments and it read 'sting' as a path. Verified with 'pre-commit run codespell'.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #540 +/- ##
==========================================
+ Coverage 97.97% 98.00% +0.02%
==========================================
Files 12 12
Lines 1281 1300 +19
==========================================
+ Hits 1255 1274 +19
Misses 26 26 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
Codecov found the branch uncovered: no subject in the suite led with an -ies word, so 'tries' -> 'try' was never taken.
Sorry, something went wrong.
The IMPERATIVES fast path was protecting exactly four words -- embed, feed, ping, speed -- and all four are already in NON_IMPERATIVE_LOOKALIKES, so it changed no verdict. Checked over the 2,333 distinct first words in git.git's history: zero differences, 576 rejections either way. Removing it leaves the morphology as the only thing that decides, and moves the lookalike check into _is_inflected where it belongs, since those words exist because of the suffix test rather than beside it. IMPERATIVES stays for the -s stem test, which genuinely needs it, but its header no longer tells contributors to treat a rejected subject as a missing entry. That contract is what #526 was about, and it is gone: an absent verb now costs a missed violation, never a false rejection.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@commit_check/imperatives.py`: - Around line 5-13: Audit the morphology vocabularies in commit_check/imperatives.py as one contract: at lines 5-13, remove non-verb entries such as partial and setup so the third-person stem set cannot classify partials or setups as verbs; at lines 548-579, add valid imperative forms including sling, weed, and wing, and add regression tests covering both the false-rejection cases and the newly recognized verbs.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dc3decc-abce-4a7e-8323-c5a741f5ba71
📥 CommitsReviewing files that changed from the base of the PR and between c6fbd53 and eb546e5.
📒 Files selected for processing (4)
Sorry, something went wrong.
Two real false rejections, both found in review. Non-verbs in IMPERATIVES now reject plural nouns, because the set is a stem oracle rather than a gate: 'partial' and 'setup' made 'partials' and 'setups' read as third-person verbs. Removed, along with 'auto'. Kept 'init', 'polyfill' and 'abstract' -- those are verbs people really do write, and their plurals are the same irreducible ambiguity as 'tests'. NON_IMPERATIVE_LOOKALIKES was missing members of both families, so 'weed out the dead code' failed. Rather than add the three that came up, enumerate the rest of both closed sets: heed, wed, weed, and ding, sling, wing, zing. A half-listed closed set is the 'add my word' treadmill again, in miniature. git.git corpus: 574 rejected, from 576.
|
Sorry, something went wrong.
* ci: check this pull request with the commit-check in it The workflow ran commit-check-action, which installs a released commit-check -- so every pull request was checked by the version before it. #540 had its own title rejected by the bug it was fixing, and the same thing happened tonight in the .github repository. A self-test that cannot see the change under test is not a self-test, and the action's version does not track the engine's, so a fix needed two releases and a bump before it could be reproduced here. Install from the checkout instead. The cost is job-summary and pr-comments, which are the action's rendering rather than the engine's behaviour and are covered by the action's own repository. Enumerating the commits is not optional. On a pull_request checkout HEAD is the synthetic merge commit, which the engine skips, so a bare 'commit-check --message' reports a pass having read nothing. Verified on a scratch repository holding one plainly bad commit: the bare form exits 0, this workflow exits 1. HEAD^1..HEAD^2 gives the real commits, and a guard fails loudly if HEAD is not a merge so the failure can never be silence. The pull request title reaches the CLI through the environment. Titles are attacker-controlled, and interpolating one into a run: block is a script injection. * ci: install with the flags the other workflows already use SonarCloud's unpinned-install rule failed the quality gate on a bare 'pip install .'. main.yml and publish-package.yml both carry --only-binary :all: for exactly this, added in #479; this step was the one that did not. Verified it still installs a local path: the resulting version reads 2.13.4.post1.dev4, which is the checkout rather than the release. * ci: close stdin so the branch and author checks cannot hang With stdin left open, commit-check waits to read a message even when only --branch and --author-name were asked for. Reproduced locally: the process runs until killed, which in CI is a step that hangs rather than fails. Closing stdin also decides what the author checks look at, so the comment says which: the author of HEAD, matching what the action reports today. * ci: revert the install flags that did not do what I thought I added --only-binary :all: because main.yml and publish-package.yml carry it and #479 mentions SonarCloud. It does not satisfy this rule -- the alert stayed -- and --upgrade pip added a second flagged line, so the change took the count from one to two. Back to one plain line. The remaining alert needs a decision rather than another flag: main.yml has the identical install and passes only because it is not new code, so locking would mean a hash-pinned requirements file or reviving the stale uv.lock. * refactor: move the checks into the nox session that already existed The session was already there and already wrong: it ran a bare 'commit-check --message', which on a pull_request checkout inspects the merge commit, which the engine skips. Leaving it that way while putting a correct copy in YAML would have left two implementations, with the broken one being the one a contributor reaches for locally. So the logic lives in noxfile.py and the workflow is one line. The same command now reproduces a CI failure on a laptop, which was half the reason for moving off the action. The session adapts rather than assuming CI: HEAD^1..HEAD^2 when the checkout is a merge commit, HEAD otherwise, and the title only when PR_TITLE is set. It still refuses to pass silently -- missing HEAD^2 during a pull_request event is an error, not a fallback. Verified locally: enumerates 2 of 2 commits on a merge ref and 0 on a plain one, exits 1 on a bad title having still run the branch and author checks, exits 0 on a good one, and no longer hangs on stdin. * refactor: keep the nox session simple, and CI logic in the workflow Reverts b95acfd, which moved the pull request checks into the nox session. The session is a developer command -- run commit-check on your working copy -- and folding CI's shape into it made the simple thing complicated for no gain. The justification was wrong too. I claimed the same command would reproduce a CI failure locally, then had to branch on whether HEAD^2 exists, because locally there is no merge commit and no PR title. A command that behaves differently in the two places does not reproduce one from the other. It also introduced a bug the shell never had: filtering on .strip() dropped empty messages, which this repository rejects via allow_empty_commits = false, so such a commit would have been reported as 'HEAD is not a merge commit' instead of as the thing it is. The shell keeps them: printf 'a\0\0b\0' through 'read -r -d' yields a, empty, b. noxfile.py is now byte-identical to main. * ci: check the title, and stop checking messages that get discarded main is linear and every subject ends in (#N): this repository squashes, so the commits on a branch never reach it. The title becomes the subject. Checking each commit was protecting history that does not exist, and cost thirty lines of shell to do it. What is left is two commands. The bare 'commit-check --message' still cannot be one of them -- it reads HEAD, the synthetic merge commit, which the engine skips and would pass having read nothing -- so the title goes in through stdin instead, which sidesteps HEAD entirely and needs no enumeration. Contributors lose CI feedback on intermediate commit messages. The pre-commit hook is where that belongs anyway: it arrives while the message is being written rather than a round trip later. Verified on a merge ref: bad title 1, good title 0, good title with a bad branch name 1.
| Back | FazBrowse Home | New Git URL |
Closes #526.
Why the list could not work
CC003 asked whether the subject's first word appeared in a set of known imperative verbs. Imperative mood is a property of a word's form, not of its membership in a vocabulary — so the list was an approximation that rejected correct subjects wherever it fell short, and it always fell short.
#527 grew it from 396 to 529 words. That helped and did not fix it: each release recognises a few more verbs and the next contributor finds the next gap. It happened again in this session — fix: treat a skipped run as non-failing was rejected because treat had been added one release later.
The change
The rule now asks the opposite question: is the first word in a form that is not imperative?
That is also the only failure this rule was ever meant to catch, so a rejection now means the author really did write fixed.
-s is treated differently from the other two, because it is weaker evidence: plural nouns wear one too. It asks for corroboration — strip -s/-es/-ies and the stem has to be a verb already in IMPERATIVES. So fixes → fix still fails while status passes. Without that, fix: status report was rejected (caught in review).
Measured
59,140 non-merge subjects from git.git, a project that writes strictly imperative subjects:
The ~1,175 the corroboration recovers are nouns: refs×283, ls×189, vcs×111, docs×57, files×30, status×14. What still fails is mostly right — added, fixed, updated, removed, fixes.
Known limit, stated rather than hidden: the residue still contains untracked×26, packed×17, spelling×14, detached×12 — participles and gerund-nouns leading a noun phrase ("spelling in the docs"). Morphology cannot separate those from real inflections without a dictionary, and main rejects them too, so it is a pre-existing limit and not a regression. notes and tests are read as verbs for the same reason.
What happens to IMPERATIVES
It is no longer an allow-list, and nothing passes or fails by being in it. The first_word in IMPERATIVES fast path turned out to guard exactly four words — embed, feed, ping, speed — all of which are already in NON_IMPERATIVE_LOOKALIKES, so it decided nothing. Removed, and verified across the 2,333 distinct first words in git.git: zero verdict differences.
What the set is still needed for is the stem test behind the -s rule. Dropping it entirely would mean no -s detection at all, costing fixes, updates and checks — the exact examples CC003's own error text and the docs name. So it stays, in a smaller job.
That change of job has a consequence found in review: an entry that is not a verb now turns its plural into a false rejection. partial, setup and auto were making partials, setups and autos read as third-person verbs, and have been removed. init, polyfill and abstract were checked and kept — those are verbs people write.
The practical consequence, and the one #526 was really about: there is nothing to keep up with any more. A verb missing from the set now costs a missed violation, never a false rejection, so nobody needs to send a patch adding the verb they just used. The file header said the opposite; it has been rewritten.
NON_IMPERATIVE_LOOKALIKES (new, 31 words) is the one set that still wants care, and only for words ending in -ed or -ing without being one. Both families are enumerated completely rather than sampled — a half-listed closed set would rebuild the same treadmill in miniature.
The deliberate loosening
A noun-led subject such as fix: parser improvements now passes, where the list rejected it — by accident of vocabulary, not by detecting the mood. test_a_noun_led_subject_now_passes names that case so anyone tightening it later does so on purpose.
Behaviour changes in the loosening direction only: subjects that used to fail now pass, so no one's build breaks on upgrade.
Note on the .pre-commit-config.yaml change
It is not unrelated, despite looking it. This PR adds sting to NON_IMPERATIVE_LOOKALIKES, and codespell reads that as a typo for string — build fails without the exclusion. The iterm entry was already there; the line is only re-quoted because an unquoted comma in a YAML flow sequence splits iterm,sting into two arguments, which is why the first attempt at the exclusion silently did nothing.
Verification
Docs
None needed — commit-check.com already described CC003 as checking for "fix, not fixed, fixes, or fixing". The documentation was describing this behaviour all along; the implementation just didn't match it.
Per #526 this probably wants a minor version and a changelog note.
🤖 Generated with Claude Code
https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn