Skip to content

feat: check imperative mood by form rather than by vocabulary - #540

Merged
shenxianpeng merged 6 commits into
mainfrom
claude/imperative-morphology-602anc
Aug 8, 2026
Merged

feat: check imperative mood by form rather than by vocabulary#540
shenxianpeng merged 6 commits into
mainfrom
claude/imperative-morphology-602anc

Conversation

@shenxianpeng

@shenxianpeng shenxianpeng commented Aug 8, 2026

Copy link
Copy Markdown
Member

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?

  • past tense — fixed, updated, removed
  • gerund — adding, implementing
  • third-person singular — fixes, removes

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:

rule rejected
allow list (529 words, main) 26,924 (45.5%)
morphology, without corroboration 1,751 (2.96%)
this branch 574 (0.97%)

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 stringbuild 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

  • 564 passed, ruff clean and formatted, pre-commit run --all-files clean.
  • Tests in TestImperativeMorphology; reverting the rule turns 10 red, reverting just the -s corroboration turns 5 red — both checked by restoring the old code.
  • The fast-path removal is proven behaviour-preserving on the corpus rather than argued.
  • One pre-existing failure is unrelated and untouched: config_test.py::test_load_config_file_permission_error fails on a clean main in 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, 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

@shenxianpeng
shenxianpeng requested a review from a team as a code owner August 8, 2026 12:07
@github-actions github-actions Bot added the enhancement New feature or request label Aug 8, 2026
@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

SubjectImperativeValidator 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.

Changes

Imperative morphology validation

Layer / File(s) Summary
Morphology contract and validator
commit_check/imperatives.py, commit_check/engine.py
Adds NON_IMPERATIVE_LOOKALIKES and rejects first words ending in ed, ing, or recognized singular s forms. Other words pass.
Morphology validation coverage
tests/engine_test.py, .pre-commit-config.yaml
Tests inflected-word rejection, valid imperative and adverb-led subjects, spelling variants, noun-led subjects, lookalike exceptions, and merge or fixup bypasses. Documents codespell exclusions.

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
Loading

Possibly related PRs

Suggested labels: tests

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The .pre-commit codespell exclusions for "iterm" and "sting" are unrelated to the imperative-mood objectives in [#526]. Remove the unrelated .pre-commit changes or provide a linked issue and clear rationale for including them.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The implementation addresses the morphology-based validation objectives in [#526], including inflected forms, exceptions, and broader valid subjects.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: detecting imperative mood by word form instead of vocabulary membership.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/imperative-morphology-602anc

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

Commit Check

All 10 checks passed

Show all 10 checks
Commit 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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 61bd994 and c6fbd53.

📒 Files selected for processing (3)
  • commit_check/engine.py
  • commit_check/imperatives.py
  • tests/engine_test.py

Comment thread commit_check/engine.py Outdated
Comment thread tests/engine_test.py Outdated
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'.
@shenxianpeng
shenxianpeng force-pushed the claude/imperative-morphology-602anc branch from c6fbd53 to 709b823 Compare August 8, 2026 12:16
@shenxianpeng shenxianpeng changed the title feat: decide imperative mood by form rather than by vocabulary feat: check imperative mood by form rather than by vocabulary Aug 8, 2026
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

codecov Bot commented Aug 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.00%. Comparing base (61bd994) to head (0b82435).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Codecov found the branch uncovered: no subject in the suite led with an
-ies word, so 'tries' -> 'try' was never taken.
@codspeed-hq

codspeed-hq Bot commented Aug 8, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 449 untouched benchmarks
🆕 40 new benchmarks
⏩ 121 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
🆕 test_a_noun_led_subject_now_passes N/A 386.9 µs N/A
🆕 test_inflected_first_words_are_rejected[docs: documents the API] N/A 611.6 µs N/A
🆕 test_inflected_first_words_are_rejected[feat: adding a retry] N/A 589.7 µs N/A
🆕 test_inflected_first_words_are_rejected[feat: implementing the cache] N/A 587.2 µs N/A
🆕 test_inflected_first_words_are_rejected[fix: fixed the parser] N/A 591.6 µs N/A
🆕 test_inflected_first_words_are_rejected[fix: fixes the parser] N/A 623.2 µs N/A
🆕 test_inflected_first_words_are_rejected[fix: removed the flag] N/A 586.8 µs N/A
🆕 test_inflected_first_words_are_rejected[fix: removes the flag] N/A 615.2 µs N/A
🆕 test_inflected_first_words_are_rejected[fix: updated the docs] N/A 589.6 µs N/A
🆕 test_lookalikes_are_not_mistaken_for_inflection[chore: spread the load] N/A 390.9 µs N/A
🆕 test_lookalikes_are_not_mistaken_for_inflection[feat: bring back the flag] N/A 383.9 µs N/A
🆕 test_lookalikes_are_not_mistaken_for_inflection[fix: embed the token] N/A 392.4 µs N/A
🆕 test_lookalikes_are_not_mistaken_for_inflection[fix: need a newer pip] N/A 387.4 µs N/A
🆕 test_lookalikes_are_not_mistaken_for_inflection[fix: ping the endpoint] N/A 383.2 µs N/A
🆕 test_lookalikes_are_not_mistaken_for_inflection[fix: proceed without the cache] N/A 391.9 µs N/A
🆕 test_lookalikes_are_not_mistaken_for_inflection[fix: string the parts together] N/A 386.2 µs N/A
🆕 test_merge_and_fixup_subjects_still_bypass_the_rule N/A 399.1 µs N/A
🆕 test_uninflected_first_words_pass_without_being_listed[feat: optionally skip the hook] N/A 395.5 µs N/A
🆕 test_uninflected_first_words_pass_without_being_listed[fix: explicitly close the handle] N/A 395.4 µs N/A
🆕 test_uninflected_first_words_pass_without_being_listed[refactor: normalise the path separators] N/A 394 µs N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.


Comparing claude/imperative-morphology-602anc (0b82435) with main (16d328f)2

Open in CodSpeed

Footnotes

  1. 121 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on main (61bd994) during the generation of this report, so 16d328f was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between c6fbd53 and eb546e5.

📒 Files selected for processing (4)
  • .pre-commit-config.yaml
  • commit_check/engine.py
  • commit_check/imperatives.py
  • tests/engine_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/engine_test.py
  • commit_check/engine.py

Comment thread commit_check/imperatives.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.
@sonarqubecloud

sonarqubecloud Bot commented Aug 8, 2026

Copy link
Copy Markdown

@shenxianpeng
shenxianpeng merged commit a90e8c8 into main Aug 8, 2026
29 checks passed
@shenxianpeng
shenxianpeng deleted the claude/imperative-morphology-602anc branch August 8, 2026 18:46
shenxianpeng added a commit that referenced this pull request Aug 9, 2026
* 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.
@shenxianpeng shenxianpeng added the minor A minor version bump label Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request minor A minor version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CC003 rejects correct imperative subjects: a word list cannot decide imperative mood

1 participant