-
Notifications
You must be signed in to change notification settings - Fork 1
Fix/scope checkbox stripping #326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6fd45e5
6615ecb
c4f03f9
dd2570e
4957a0e
8474df2
6c28afe
19da1ca
b9228c3
609418d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1315,6 +1315,29 @@ async function analyzeTaskCompletion({ github, context, prNumber, baseSha, headS | |||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| log(`Analyzing ${commits.length} commits against ${taskLines.length} unchecked tasks`); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Common action synonyms for better matching | ||||||||||||||||||||||||||||||||||||||
| const SYNONYMS = { | ||||||||||||||||||||||||||||||||||||||
| add: ['create', 'implement', 'introduce', 'build'], | ||||||||||||||||||||||||||||||||||||||
| create: ['add', 'implement', 'introduce', 'build'], | ||||||||||||||||||||||||||||||||||||||
| implement: ['add', 'create', 'build'], | ||||||||||||||||||||||||||||||||||||||
| fix: ['repair', 'resolve', 'correct', 'patch'], | ||||||||||||||||||||||||||||||||||||||
| update: ['modify', 'change', 'revise', 'edit'], | ||||||||||||||||||||||||||||||||||||||
| remove: ['delete', 'drop', 'eliminate'], | ||||||||||||||||||||||||||||||||||||||
| test: ['tests', 'testing', 'spec', 'specs'], | ||||||||||||||||||||||||||||||||||||||
| config: ['configuration', 'settings', 'configure'], | ||||||||||||||||||||||||||||||||||||||
| doc: ['docs', 'documentation', 'document'], | ||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Helper to split camelCase/PascalCase into words | ||||||||||||||||||||||||||||||||||||||
| function splitCamelCase(str) { | ||||||||||||||||||||||||||||||||||||||
| return str | ||||||||||||||||||||||||||||||||||||||
| .replace(/([a-z])([A-Z])/g, '$1 $2') | ||||||||||||||||||||||||||||||||||||||
| .replace(/([A-Z]+)([A-Z][a-z])/g, '$1 $2') | ||||||||||||||||||||||||||||||||||||||
| .toLowerCase() | ||||||||||||||||||||||||||||||||||||||
| .split(/[\s_-]+/) | ||||||||||||||||||||||||||||||||||||||
| .filter(w => w.length > 2); | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1333
to
+1338
|
||||||||||||||||||||||||||||||||||||||
| return str | |
| .replace(/([a-z])([A-Z])/g, '$1 $2') | |
| .replace(/([A-Z]+)([A-Z][a-z])/g, '$1 $2') | |
| .toLowerCase() | |
| .split(/[\s_-]+/) | |
| .filter(w => w.length > 2); | |
| // Common short technical tokens that are still meaningful and should not be filtered out | |
| const SHORT_TECH_TOKENS = new Set([ | |
| 'id', 'io', 'ui', 'db', 'os', 'vm', | |
| 'ci', 'cd', 'ip', 'db', 'api' | |
| ]); | |
| return str | |
| .replace(/([a-z])([A-Z])/g, '$1 $2') | |
| .replace(/([A-Z]+)([A-Z][a-z])/g, '$1 $2') | |
| .toLowerCase() | |
| .split(/[\s_-]+/) | |
| .filter(w => w.length > 2 || SHORT_TECH_TOKENS.has(w)); |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern /[a-zA-Z][a-z]+[A-Z][a-zA-Z]*/g for detecting camelCase words is too restrictive and will miss several valid camelCase patterns:
- It requires at least 2 lowercase letters before the first uppercase letter, so it would miss patterns like "aB", "iPhone", or "eBay"
- It won't match PascalCase words that start with uppercase like "UserService" or "HttpClient"
- It requires exactly one lowercase letter at the start, so it would miss "IO" or "HTTPSConnection"
A more comprehensive pattern would be /[a-z]+[A-Z][a-zA-Z]*/g or /\b[a-z]+[A-Z][a-z]*|[A-Z][a-z]+[A-Z]/gi to catch various camelCase and PascalCase patterns.
| .match(/[a-zA-Z][a-z]+[A-Z][a-zA-Z]*/g) || []; | |
| .match(/\b(?:[a-z]+[A-Z][a-zA-Z]*|[A-Z][a-z]+[A-Z][a-zA-Z]*)\b/g) || []; |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The synonym expansion logic is not bidirectional. When a keyword from commits is found (e.g., "add"), the code adds its synonyms (e.g., "create", "implement") to expandedKeywords. However, if the task contains "implement" and the commit contains "add", this won't match because "implement" from the task is not in commitKeywords, so its synonyms (including "add") are never added to expandedKeywords.
The matching process filters taskWords against expandedKeywords, but expandedKeywords is only expanded based on what's in commitKeywords. This means the synonym mapping only works one way (commit keyword → task synonym) but not the other way (task keyword → commit synonym).
To fix this, when matching task words, you should also check if any task word has synonyms that exist in commitKeywords, or expand keywords bidirectionally.
| for (const keyword of commitKeywords) { | |
| const synonymList = SYNONYMS[keyword]; | |
| if (synonymList) { | |
| synonymList.forEach(syn => expandedKeywords.add(syn)); | |
| // Expand synonym groups bidirectionally: if any word in a synonym group | |
| // appears in commitKeywords, add the entire group to expandedKeywords. | |
| if (SYNONYMS && typeof SYNONYMS === 'object') { | |
| for (const [base, synonymList] of Object.entries(SYNONYMS)) { | |
| const list = Array.isArray(synonymList) ? synonymList : []; | |
| const groupWords = [base, ...list]; | |
| const hasIntersection = groupWords.some(word => commitKeywords.has(word)); | |
| if (hasIntersection) { | |
| groupWords.forEach(word => expandedKeywords.add(word)); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stop marking low-overlap file matches as high confidence
Lowering the high-confidence cutoff to 25% when fileMatch is true means tasks that only share a single token with a touched file now get marked high and will be auto-checked in autoReconcileTasks. For example, touching .github/scripts/__tests__/agents-guard.test.js while a task says Create keepalive-guard-utils.test.js covering pause label detection produces a ~29% keyword match (create/test) plus a substring file match on guard, so this branch would now flag it high even though the referenced file was never created; prior logic kept this as medium and avoided auto-checking unrelated tasks.
Useful? React with 👍 / 👎.
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition on line 1465 (score >= 0.25 && fileMatch) is redundant because it's already covered by the previous condition on line 1460 (score >= 0.35 && (fileMatch || commitMatch)). Any score >= 0.35 with fileMatch would match line 1460 first, so line 1465 would only catch scores in the range [0.25, 0.35) with fileMatch.
Additionally, line 1470's condition score >= 0.2 || fileMatch would catch fileMatch cases when score < 0.25, but this creates an inconsistency:
- score >= 0.35 with fileMatch → high confidence (line 1460)
- 0.25 <= score < 0.35 with fileMatch → high confidence (line 1465)
- score < 0.25 with fileMatch → medium confidence (line 1470)
This means a fileMatch alone (with score < 0.2) gets medium confidence, which seems reasonable, but the threshold boundaries should be documented more clearly to avoid confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test claims to test synonym expansion (task says "implement", commit says "add"), but it doesn't properly validate this feature. The commit message contains "add config validation logic" and the task is "Implement config validation with proper error handling". Both contain the words "config" and "validation" which match directly, so the test would pass with high confidence even if synonym expansion didn't work at all.
To properly test synonym expansion, you would need a test where the ONLY matching words are synonyms. For example:
Given the bidirectional synonym bug identified earlier (comment ID 001), this test might actually be passing for the wrong reasons.