fix(history): count all imperative samples - #248
Conversation
Include descriptive verb forms in the imperative-rate denominator. Closes #204
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesImperative Rate Classification
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdb1b7e302
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| imperativeSampleCount++; | ||
| if (!verb.endsWith('ed') && !verb.endsWith('ing')) { | ||
| imperativeCount++; |
There was a problem hiding this comment.
When history contains base-form imperative verbs whose spelling ends in ed or ing (such as seed or bring), incrementing the denominator here while excluding those verbs from the numerator misclassifies them as descriptive. For fix: add retries, fix: bring retries, and fix: added retries, the rate becomes 1/3 instead of the correct 2/3, so buildStyleGuidance() no longer emits imperative guidance at its 0.5 threshold; classify the verb form rather than relying solely on its suffix before counting the sample.
AGENTS.md reference: AGENTS.md:L63-L63
Useful? React with 👍 / 👎.
Add a set of base-form verbs that end with 'ed' or 'ing' (e.g., 'bleed', 'bring') to avoid counting them as descriptive in the imperative rate calculation. Refactor verb extraction to use optional chaining and fallback. Add test to verify correct behavior.
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/history/store.ts`:
- Around line 42-43: Normalize the verb to lowercase at the start of
isDescriptiveVerb before both BASE_FORM_VERBS_WITH_SUFFIXES lookup and suffix
checks. Update tests/history-profile.test.mjs lines 70-85 to use capitalized
“Bring” in the fixture while retaining the expected 2 / 3 imperative rate.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1d61a79f-baa2-4197-9722-f77fde2ec7b5
📒 Files selected for processing (2)
src/history/store.tstests/history-profile.test.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
The project is ESM-only: use
.jsextensions in local imports and keepverbatimModuleSyntax-compatible TypeScript imports (for example,import typefor type-only imports).
Files:
src/history/store.ts
src/{history,llm}/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Recent commit history should be stored as JSONL, analyzed by
buildProfile()for commit-style patterns, and injected into prompts as style guidance.
Files:
src/history/store.ts
tests/**/*.mjs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.mjs: Use Node.js built-innode:testfor test files intests/andtests/e2e/; do not use Jest or Mocha.
Write test assertions withnode:assert/strictin test files undertests/andtests/e2e/.
Files:
tests/history-profile.test.mjs
🔇 Additional comments (2)
src/history/store.ts (1)
348-352: LGTM!tests/history-profile.test.mjs (1)
46-68: LGTM!
| function isDescriptiveVerb(verb: string): boolean { | ||
| return !BASE_FORM_VERBS_WITH_SUFFIXES.has(verb) && (verb.endsWith('ed') || verb.endsWith('ing')); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize verb casing before suffix classification.
verb preserves commit-message casing, but the exception set is lowercase. Therefore fix: Bring retries is incorrectly treated as descriptive and lowers the imperative rate.
src/history/store.ts#L42-L43: normalizeverbbefore both the exception lookup and suffix checks.tests/history-profile.test.mjs#L70-L85: capitalizebringin the fixture (or add a capitalized case) and retain the expected2 / 3rate.
Proposed fix
function isDescriptiveVerb(verb: string): boolean {
- return !BASE_FORM_VERBS_WITH_SUFFIXES.has(verb) && (verb.endsWith('ed') || verb.endsWith('ing'));
+ const normalizedVerb = verb.toLowerCase();
+ return !BASE_FORM_VERBS_WITH_SUFFIXES.has(normalizedVerb)
+ && (normalizedVerb.endsWith('ed') || normalizedVerb.endsWith('ing'));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isDescriptiveVerb(verb: string): boolean { | |
| return !BASE_FORM_VERBS_WITH_SUFFIXES.has(verb) && (verb.endsWith('ed') || verb.endsWith('ing')); | |
| function isDescriptiveVerb(verb: string): boolean { | |
| const normalizedVerb = verb.toLowerCase(); | |
| return !BASE_FORM_VERBS_WITH_SUFFIXES.has(normalizedVerb) | |
| && (normalizedVerb.endsWith('ed') || normalizedVerb.endsWith('ing')); | |
| } |
📍 Affects 2 files
src/history/store.ts#L42-L43(this comment)tests/history-profile.test.mjs#L70-L85
🤖 Prompt for 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.
In `@src/history/store.ts` around lines 42 - 43, Normalize the verb to lowercase
at the start of isDescriptiveVerb before both BASE_FORM_VERBS_WITH_SUFFIXES
lookup and suffix checks. Update tests/history-profile.test.mjs lines 70-85 to
use capitalized “Bring” in the fixture while retaining the expected 2 / 3
imperative rate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a355bbdd5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ]); | ||
|
|
||
| function isDescriptiveVerb(verb: string): boolean { | ||
| return !BASE_FORM_VERBS_WITH_SUFFIXES.has(verb) && (verb.endsWith('ed') || verb.endsWith('ing')); |
There was a problem hiding this comment.
When history contains sentence-case subjects such as Ping server, the extracted verb is Ping, which misses the lowercase ping entry and is classified as descriptive solely because it ends in ing. Repositories using capitalized subjects therefore get an artificially low imperative rate, potentially suppressing the imperative-mood prompt when the rate falls below 50%; lowercase the verb before both checks.
AGENTS.md reference: AGENTS.md:L63-L63
Useful? React with 👍 / 👎.
| ]); | ||
|
|
||
| function isDescriptiveVerb(verb: string): boolean { | ||
| return !BASE_FORM_VERBS_WITH_SUFFIXES.has(verb) && (verb.endsWith('ed') || verb.endsWith('ing')); |
There was a problem hiding this comment.
When an imperative subject begins with a compound base verb such as fix: reseed database, reseed ends in ed but is absent from the exception set even though its root seed is present, so the new denominator logic records it as descriptive. The same applies to forms such as preseed and valid base verbs such as shred, causing the learned tone and resulting prompt guidance to be wrong for those histories; the exception logic needs to recognize these base forms rather than relying on this incomplete list.
AGENTS.md reference: AGENTS.md:L63-L63
Useful? React with 👍 / 👎.
The lowercase conversion was missing when checking if a verb ends with 'ed' or 'ing', causing capitalized verbs like 'Bring' to be incorrectly classified as descriptive verbs.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9973b1dea5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function isDescriptiveVerb(verb: string): boolean { | ||
| verb = verb.toLowerCase(); | ||
| return !BASE_FORM_VERBS_WITH_SUFFIXES.has(verb) && (verb.endsWith('ed') || verb.endsWith('ing')); |
There was a problem hiding this comment.
When a commit uses an imperative derived verb such as fix: reseed database, the exact allowlist does not contain reseed, so the ed suffix incorrectly marks it as descriptive. For example, a history containing that subject and fix: added retry produces an imperative rate of 0 instead of 50%, preventing buildStyleGuidance() from preserving the repository's imperative style. Prefix-derived forms such as reseed and preseed need classification beyond an exact-word allowlist.
Useful? React with 👍 / 👎.
|
1 participant
Summary
Corrects style-profile tone detection so descriptive commit verbs are included in the imperative-rate denominator.
Changes
Testing
npm run buildnode --test tests/history-profile.test.mjsRelated
Closes #204
Summary by CodeRabbit
Bug Fixes
Tests