feat: check imperative mood by form rather than by vocabulary - #540
Conversation
📝 WalkthroughWalkthrough
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: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Commit Check✅ All 10 checks passed Show all 10 checkscommit-check 2.13.4 · Rules reference |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify 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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aba3f9c5-171c-45e7-8243-a9985fe1244e
📒 Files selected for processing (3)
commit_check/engine.pycommit_check/imperatives.pytests/engine_test.py
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'.
c6fbd53 to
709b823
Compare
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. Additional details and impacted files@@ 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. 🚀 New features to boost your workflow:
|
Codecov found the branch uncovered: no subject in the suite led with an -ies word, so 'tries' -> 'try' was never taken.
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 agents
Verify 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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dc3decc-abce-4a7e-8323-c5a741f5ba71
📒 Files selected for processing (4)
.pre-commit-config.yamlcommit_check/engine.pycommit_check/imperatives.pytests/engine_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/engine_test.py
- commit_check/engine.py
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.
|
* 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.



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-failingwas rejected becausetreathad 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?
fixed,updated,removedadding,implementingfixes,removesThat is also the only failure this rule was ever meant to catch, so a rejection now means the author really did write
fixed.-sis treated differently from the other two, because it is weaker evidence: plural nouns wear one too. It asks for corroboration — strip-s/-es/-iesand the stem has to be a verb already inIMPERATIVES. Sofixes → fixstill fails whilestatuspasses. Without that,fix: status reportwas rejected (caught in review).Measured
59,140 non-merge subjects from
git.git, a project that writes strictly imperative subjects:main)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, andmainrejects them too, so it is a pre-existing limit and not a regression.notesandtestsare read as verbs for the same reason.What happens to
IMPERATIVESIt is no longer an allow-list, and nothing passes or fails by being in it. The
first_word in IMPERATIVESfast path turned out to guard exactly four words —embed,feed,ping,speed— all of which are already inNON_IMPERATIVE_LOOKALIKES, so it decided nothing. Removed, and verified across the 2,333 distinct first words ingit.git: zero verdict differences.What the set is still needed for is the stem test behind the
-srule. Dropping it entirely would mean no-sdetection at all, costingfixes,updatesandchecks— 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,setupandautowere makingpartials,setupsandautosread as third-person verbs, and have been removed.init,polyfillandabstractwere 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-edor-ingwithout 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 improvementsnow passes, where the list rejected it — by accident of vocabulary, not by detecting the mood.test_a_noun_led_subject_now_passesnames 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.yamlchangeIt is not unrelated, despite looking it. This PR adds
stingtoNON_IMPERATIVE_LOOKALIKES, and codespell reads that as a typo forstring—buildfails without the exclusion. Theitermentry was already there; the line is only re-quoted because an unquoted comma in a YAML flow sequence splitsiterm,stinginto two arguments, which is why the first attempt at the exclusion silently did nothing.Verification
564 passed, ruff clean and formatted,pre-commit run --all-filesclean.TestImperativeMorphology; reverting the rule turns 10 red, reverting just the-scorroboration turns 5 red — both checked by restoring the old code.config_test.py::test_load_config_file_permission_errorfails on a cleanmainin this environment too, because the suite runs as root and a permission-denied path cannot be provoked.Docs
None needed — commit-check.com already described CC003 as checking for "
fix, notfixed,fixes, orfixing". 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