fix(extraction): reduce injection noise — checkbox filter + narrative quality scoring (#68)#158
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughДобавлена детекция переключений чекбоксов задач и исключение их из извлечения; интегрировано в пайплайн классификации событий. Добавлен множитель качества повествования в расчёт оценок и соответствующие юнит-тесты. Текстовые системные промты расширены для игнорирования таких чекбокс‑изменений. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements logic to ignore routine task checkbox toggles during observation extraction and introduces a narrative quality multiplier to penalize thin descriptions in scoring. Review feedback suggests improving the robustness of the checkbox detection regex and ensuring file path matching is performed on the actual path rather than the full JSON input to avoid false positives. There is also a suggestion to use word boundaries for reasoning markers to prevent incorrect substring matches.
| if !taskFileRe.MatchString(toolInput) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
The current implementation matches the taskFileRe against the entire toolInput JSON string. This can lead to false positives if a task-related filename (e.g., tasks.md) appears within the content of an edit for a non-task file (e.g., a comment in main.go referencing the task list). It is safer to extract the actual file path being edited and verify it against the regex.
| if !taskFileRe.MatchString(toolInput) { | |
| return false | |
| } | |
| // Check if file is a task/checklist file | |
| path := extractFilePath(toolInput) | |
| if path == "" || !taskFileRe.MatchString(path) { | |
| return false | |
| } |
| ) | ||
|
|
||
| // checkboxToggleRe matches checkbox state changes in task lists. | ||
| var checkboxToggleRe = regexp.MustCompile(`-\s*\[[ x]\]\s`) |
There was a problem hiding this comment.
The regex for detecting checkboxes is slightly too restrictive. It should support uppercase [X], which is common in many Markdown flavors. Additionally, requiring a trailing whitespace character (\s) might fail to match checkboxes at the end of a string or followed by JSON delimiters (like a closing quote) when matching against the raw toolInput string. Consider allowing the end of the string or a broader set of delimiters.
| var checkboxToggleRe = regexp.MustCompile(`-\s*\[[ x]\]\s`) | |
| var checkboxToggleRe = regexp.MustCompile(`(?i)-\s*\[[ x]\](\s|$)`) |
| for _, marker := range reasoningMarkers { | ||
| if strings.Contains(lower, marker) { | ||
| return 1.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Using strings.Contains with short markers like "why" or "reason" can lead to false positives by matching substrings in unrelated words (e.g., "anywhere" contains "where", though "why" is safer; however, "reason" matches "treason" or "unreasonable"). While acceptable as a heuristic, using word boundaries or a more specific check would improve the accuracy of the reasoning detection.
![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
🧹 Nitpick comments (3)
internal/scoring/calculator.go (1)
213-216: Проверка маркеров черезContainsдаёт ложные срабатывания по подстрокам.Это может необоснованно поднимать множитель до
1.0и снижать эффективность фильтра шума. Лучше матчить слова/фразы с границами.Вариант исправления
import ( "math" + "regexp" "strings" "time" "github.com/thebtf/engram/pkg/models" ) @@ -var reasoningMarkers = []string{ - "because", "chose", "decided", "instead of", "rather than", - "tradeoff", "trade-off", "alternative", "reason", "why", -} +var reasoningMarkerRE = regexp.MustCompile(`(?i)\b(because|chose|decided|tradeoff|trade-off|alternative|reason|why)\b|instead of|rather than`) @@ - lower := strings.ToLower(narrative) - for _, marker := range reasoningMarkers { - if strings.Contains(lower, marker) { - return 1.0 - } - } + if reasoningMarkerRE.MatchString(narrative) { + return 1.0 + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scoring/calculator.go` around lines 213 - 216, The current loop uses strings.Contains on lower (lowercased narrative) against reasoningMarkers which causes substring false-positives; change the check to match whole words/phrases using word-boundary matching (e.g., build a regex per marker with \b + regexp.QuoteMeta(marker) + \b and test with MatchString on lower) or tokenize the narrative and compare tokens/phrases exactly; update the loop that iterates over reasoningMarkers (and any helper that uses lower/narrative) to use this boundary-safe match so only full-word/phrase hits return 1.0.internal/scoring/calculator_test.go (1)
786-797: Добавьте boundary-кейсы для ровно 5 и 20 слов.Сейчас не покрыты ключевые границы условий (
<5и<20), поэтому возможна тихая регрессия порогов.Предлагаемый патч
tests := []struct { name string narrative string want float64 }{ {"very thin", "Marked T015 complete", 0.5}, + {"exactly 5 words no marker", "one two three four five", 0.7}, {"thin with marker", "Chose sync.Map because read-heavy", 1.0}, {"medium no marker", "Updated the configuration file with new settings for the deployment pipeline across environments", 0.7}, + {"exactly 20 words no marker", strings.TrimSpace(strings.Repeat("word ", 20)), 1.0}, {"long narrative", strings.Repeat("word ", 50), 1.0}, {"empty", "", 0.5}, {"short with reason", "Changed approach because the original design had a fundamental flaw in concurrency handling", 1.0}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scoring/calculator_test.go` around lines 786 - 797, Add two boundary test cases to the tests slice in calculator_test.go to cover exactly 5 and exactly 20-word narratives: add an entry (e.g., name "five words") with a 5-word string and expected want 0.7, and an entry (e.g., name "twenty words") with a 20-word string and expected want 0.7; place them alongside the existing table-driven tests so the scoring logic that handles the <5 and <20 thresholds (the tests variable in calculator_test.go) is exercised.internal/pipeline/deterministic_test.go (1)
1360-1383: Добавьте негативный кейс “чекбокс есть, но toggle нет”.Сейчас нет проверки для сценария
old_string == new_string(например,- [ ]→- [ ]), из-за чего ложноположительный детект не будет пойман регрессией.🧪 Минимальное расширение теста
func TestIsCheckboxToggle(t *testing.T) { tests := []struct { name string toolName string input string want bool }{ {"checkbox toggle in tasks.md", "Edit", `{"file_path":"tasks.md","old_string":"- [ ] T015","new_string":"- [x] T015"}`, true}, {"uncheck in tasks.md", "Edit", `{"file_path":"tasks.md","old_string":"- [x] T015","new_string":"- [ ] T015"}`, true}, + {"checkbox unchanged in tasks.md", "Edit", `{"file_path":"tasks.md","old_string":"- [ ] T015","new_string":"- [ ] T015"}`, false}, {"normal edit in main.go", "Edit", `{"file_path":"main.go","old_string":"foo","new_string":"bar"}`, false}, {"checkbox in non-task file", "Edit", `{"file_path":"README.md","old_string":"- [ ] item","new_string":"- [x] item"}`, false}, {"Read tool", "Read", `{"file_path":"tasks.md"}`, false}, {"checkbox in TODO.md", "Edit", `{"file_path":"TODO.md","old_string":"- [ ] Fix bug","new_string":"- [x] Fix bug"}`, true}, {"nested path tasks.md", "Edit", `{"file_path":".agent/specs/feature/tasks.md","old_string":"- [ ] T001","new_string":"- [x] T001"}`, true}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pipeline/deterministic_test.go` around lines 1360 - 1383, Add a negative test case to TestIsCheckboxToggle to cover the scenario where a checkbox is present but not toggled (old_string == new_string) so IsCheckboxToggle does not return a false positive; specifically update the tests slice in TestIsCheckboxToggle to include an entry (toolName "Edit") with a JSON input where "file_path" is a task file (e.g., "tasks.md") and "old_string" equals "new_string" (for example both "- [ ] T015"), and assert the expected want value is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/pipeline/deterministic.go`:
- Around line 15-33: IsCheckboxToggle currently returns true if any checkbox
pattern exists in toolInput; change it to detect an actual state change by
parsing old vs new content and confirming a transition between unchecked and
checked. Modify IsCheckboxToggle (and keep using checkboxToggleRe/taskFileRe) to
extract old_string and new_string from toolInput (or accept separate old/new
inputs), add two specific regexes for unchecked (`-\s*\[\s\]\s`) and checked
(`-\s*\[x\]\s`) states, and return true only when the file is a task file
(taskFileRe) and one of old_string/new_string matches the checked pattern while
the other matches the unchecked pattern (i.e., a real toggle between states).
---
Nitpick comments:
In `@internal/pipeline/deterministic_test.go`:
- Around line 1360-1383: Add a negative test case to TestIsCheckboxToggle to
cover the scenario where a checkbox is present but not toggled (old_string ==
new_string) so IsCheckboxToggle does not return a false positive; specifically
update the tests slice in TestIsCheckboxToggle to include an entry (toolName
"Edit") with a JSON input where "file_path" is a task file (e.g., "tasks.md")
and "old_string" equals "new_string" (for example both "- [ ] T015"), and assert
the expected want value is false.
In `@internal/scoring/calculator_test.go`:
- Around line 786-797: Add two boundary test cases to the tests slice in
calculator_test.go to cover exactly 5 and exactly 20-word narratives: add an
entry (e.g., name "five words") with a 5-word string and expected want 0.7, and
an entry (e.g., name "twenty words") with a 20-word string and expected want
0.7; place them alongside the existing table-driven tests so the scoring logic
that handles the <5 and <20 thresholds (the tests variable in
calculator_test.go) is exercised.
In `@internal/scoring/calculator.go`:
- Around line 213-216: The current loop uses strings.Contains on lower
(lowercased narrative) against reasoningMarkers which causes substring
false-positives; change the check to match whole words/phrases using
word-boundary matching (e.g., build a regex per marker with \b +
regexp.QuoteMeta(marker) + \b and test with MatchString on lower) or tokenize
the narrative and compare tokens/phrases exactly; update the loop that iterates
over reasoningMarkers (and any helper that uses lower/narrative) to use this
boundary-safe match so only full-word/phrase hits return 1.0.
🪄 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: CHILL
Plan: Pro
Run ID: a1ab31ca-6acd-4681-af34-3ba6dbcdec50
📒 Files selected for processing (6)
internal/backfill/extract/prompts.gointernal/pipeline/deterministic.gointernal/pipeline/deterministic_test.gointernal/scoring/calculator.gointernal/scoring/calculator_test.gointernal/worker/sdk/processor.go
…tection - Match taskFileRe against extracted file path, not raw JSON (Gemini) - Support uppercase [X] checkbox with case-insensitive regex (Gemini) - Detect actual state change (unchecked+checked both present), not just checkbox presence (CodeRabbit) - Use filepath.ToSlash for cross-platform path matching
1 participant
Summary
Reduces injection noise from ~80% to <30% by filtering low-value observations. Issue #68.
Changes
Deterministic checkbox filter (internal/pipeline/deterministic.go)
IsCheckboxToggle()detects tasks.md checkbox changes ([ ]→[x]) via regexClassifyEventcalls it first — checkbox edits → type=change (not decision)LLM prompt update
systemPrompt: added checkbox toggles to DO NOT EXTRACTChunkExtractionSystemPrompt: same rule for backfill extractionNarrative quality multiplier (internal/scoring/calculator.go)
narrativeQualityMultiplier(): <5 words → 0.5x, <20 words without reasoning markers → 0.7xNarrativeMultiplieradded toScoreComponentsfor explainabilityTesting
go vetcleanCloses #68 (iteration 1 — extraction quality)
Summary by CodeRabbit
Примечания к выпуску
Новые функции
Улучшения
Тесты