feat: add rejected[] field to decision observations (FR-8)#13
Conversation
Add a structured `rejected` JSONB array field to observations for storing alternatives that were considered and dismissed during decisions. This enables reliable contradiction detection without fragile narrative-text parsing. Changes: - pkg/models: add Rejected to ParsedObservation, Observation, ObservationJSON - internal/db/gorm: add Rejected column to GORM model + migration 034 - internal/mcp: add rejected param to store_memory tool schema + handler - internal/search: include rejected in observation search result metadata - plugin: update client type + after-tool-call hook to prefer structured rejected[] over regex-based narrative parsing (with fallback)
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughДобавлено поле Rejected (jsonb массив строк) во все слои: миграция БД, GORM-модель и store, публичные модели и JSON, MCP/tools (ввод), search-метаданные, плагинные типы и логика хуков для приоритизации структурированного поля. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PluginHook as Hook
participant MCP
participant Store as DBStore
participant DB
participant Search
Client->>PluginHook: вызывает after-tool-call (результат наблюдений)
PluginHook->>MCP: запрос поиска/обработки (использует rejected[] если есть)
MCP->>DBStore: StoreObservation (включая rejected[])
DBStore->>DB: INSERT/UPDATE observation (rejected jsonb)
DB->>DBStore: OK
DBStore->>MCP: сохранено
MCP->>Search: observationToResult (включая rejected)
Search->>PluginHook: результаты с metadata.rejected
PluginHook->>Client: принимает решение с приоритетом structured rejected[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ 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)
📝 Coding Plan
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.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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
The pull request successfully introduces the rejected field to decision observations, enhancing structured data capture and improving contradiction detection. The changes are consistently applied across database migrations, GORM models, API definitions, and client-side logic. The implementation includes a thoughtful fallback mechanism to narrative text parsing when the structured rejected field is empty, ensuring backward compatibility and robustness. The version bump in package.json is appropriate for this feature addition.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/db/gorm/observation_store.go (1)
646-661:⚠️ Potential issue | 🟠 MajorВ raw SQL запросах FTS отсутствует столбец
rejected.Запросы в
SearchObservationsFTS,SearchObservationsFTSFilteredиSearchObservationsFTSScoredне выбирают столбецrejected. Это приведёт к тому, что при получении наблюдений через FTS полеRejectedвсегда будет пустым, даже если данные присутствуют в базе.Также необходимо обновить функцию
scanObservationдля сканирования нового столбца.🐛 Предлагаемое исправление для запроса
ftsQuery := ` SELECT o.id, o.sdk_session_id, o.project, COALESCE(o.scope, 'project') as scope, o.type, - o.title, o.subtitle, o.facts, o.narrative, o.concepts, o.files_read, o.files_modified, + o.title, o.subtitle, o.facts, o.rejected, o.narrative, o.concepts, o.files_read, o.files_modified, o.file_mtimes, o.prompt_number, o.discovery_tokens, o.created_at, o.created_at_epoch,И аналогичное исправление для
scanObservation:func scanObservation(scanner interface{ Scan(...any) error }) (*models.Observation, error) { var obs models.Observation - var factsJSON, conceptsJSON, filesReadJSON, filesModifiedJSON, fileMtimesJSON []byte + var factsJSON, rejectedJSON, conceptsJSON, filesReadJSON, filesModifiedJSON, fileMtimesJSON []byte var isSuperseded int err := scanner.Scan( &obs.ID, &obs.SDKSessionID, &obs.Project, &obs.Scope, &obs.Type, - &obs.Title, &obs.Subtitle, &factsJSON, &obs.Narrative, &conceptsJSON, + &obs.Title, &obs.Subtitle, &factsJSON, &rejectedJSON, &obs.Narrative, &conceptsJSON, ... ) ... + if len(rejectedJSON) > 0 { + _ = json.Unmarshal(rejectedJSON, &obs.Rejected) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/gorm/observation_store.go` around lines 646 - 661, The FTS SQL queries (used by SearchObservationsFTS, SearchObservationsFTSFiltered and SearchObservationsFTSScored) do not select the rejected column so Observation.Rejected is always empty; update each FTS SELECT to include the rejected column (e.g. COALESCE(o.rejected, 0) as rejected) and then update scanObservation to expect and scan that additional column into the Observation.Rejected field (ensure the scan column order matches the SELECT and convert/scan the DB type into the Observation.Rejected boolean/zero-one representation used elsewhere).internal/mcp/tools_memory.go (1)
156-163:⚠️ Potential issue | 🟠 MajorУстановить фильтр для
Rejectedпо типу наблюдения.Поле
Rejectedимеет явный контракт в документации API (internal/mcp/server.go:1150): "Alternatives considered and dismissed (for decision observations)". Однако на строке 162 это поле передаётся безусловно для всех типов наблюдений. Требуется дополнительная проверка типа перед сохранением:obs := &models.ParsedObservation{ Type: obsType, SourceType: models.SourceManual, Title: title, Narrative: params.Content, Concepts: concepts, - Rejected: params.Rejected, + Rejected: nil, Scope: scope, } + if obsType == models.ObsTypeDecision { + obs.Rejected = params.Rejected + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/tools_memory.go` around lines 156 - 163, The ParsedObservation struct is being populated with Rejected for every observation regardless of type; change the construction in internal/mcp/tools_memory.go so that Rejected is only included when obsType indicates a decision observation (check obsType against the decision enum, e.g. models.ObservationDecision or the project’s decision constant) and otherwise set Rejected to nil/empty; update the struct literal that creates obs (ParsedObservation, field Rejected, using obsType and params.Rejected) so the Rejected field is conditional on obsType == models.ObservationDecision.
🧹 Nitpick comments (2)
plugin/openclaw-engram/src/hooks/after-tool-call.ts (1)
106-141: Снизьте ложные срабатывания в contradiction detection.Сейчас
includes(term)и fallback после паттернов (notи т.п.) легко ловят шум (подстроки/стоп-слова), что может спамить предупреждениями. Лучше проверять границы слова и фильтровать очевидные стоп-слова.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/openclaw-engram/src/hooks/after-tool-call.ts` around lines 106 - 141, The contradiction detection currently uses substring checks (contentLower.includes) in the loop over rejectedItems and in the fallback block that builds rejectedWord from rejectionPatterns, causing false positives; update both places (the rejectedItems handling and the fallback parsing logic around rejectionPatterns/rejectedWord) to match whole words or word boundaries (e.g., use a word-boundary regex like \bterm\b) instead of plain includes, and add a small stop-word filter (common short words such as "not", "instead", "rather", "avoid", "use", etc.) to skip obvious non-content tokens as well as keeping the existing minimum length check to avoid reporting on punctuation/short tokens; ensure the warning still references obs.title or the narrative excerpt as before when a true word-boundary match is found.plugin/openclaw-engram/src/client.ts (1)
126-131: Лучше вынести inline-тип ответаsearchDecisionsв именованный интерфейс.Сейчас тип в Line 130 легко рассинхронизировать с другими местами использования. Выделенный
DecisionSearchObservationупростит поддержку.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/openclaw-engram/src/client.ts` around lines 126 - 131, Create a named interface DecisionSearchObservation for the inline observation type currently used in the searchDecisions signature, replace the inline Array<{ title?: string; narrative?: string; concepts?: string[]; rejected?: string[] }> with DecisionSearchObservation[] in the Promise return type of searchDecisions, and export the interface if it will be used elsewhere; update any other references that expect the old inline type to use DecisionSearchObservation to avoid future desyncs.
🤖 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/db/gorm/migrations.go`:
- Around line 1176-1179: The migration currently adds the observations.rejected
column with DEFAULT '[]' but allows NULL; update the Up migration Exec SQL (the
string currently "ALTER TABLE observations ADD COLUMN IF NOT EXISTS rejected
JSONB DEFAULT '[]'") to include NOT NULL so the column is non-nullable (e.g.,
add "NOT NULL" after DEFAULT '[]'), and keep the Rollback Exec ("ALTER TABLE
observations DROP COLUMN IF EXISTS rejected") unchanged; ensure the change is
applied in the same migration function where the Exec call is defined so
existing rows will have the default empty array and the JSON contract remains
stable.
---
Outside diff comments:
In `@internal/db/gorm/observation_store.go`:
- Around line 646-661: The FTS SQL queries (used by SearchObservationsFTS,
SearchObservationsFTSFiltered and SearchObservationsFTSScored) do not select the
rejected column so Observation.Rejected is always empty; update each FTS SELECT
to include the rejected column (e.g. COALESCE(o.rejected, 0) as rejected) and
then update scanObservation to expect and scan that additional column into the
Observation.Rejected field (ensure the scan column order matches the SELECT and
convert/scan the DB type into the Observation.Rejected boolean/zero-one
representation used elsewhere).
In `@internal/mcp/tools_memory.go`:
- Around line 156-163: The ParsedObservation struct is being populated with
Rejected for every observation regardless of type; change the construction in
internal/mcp/tools_memory.go so that Rejected is only included when obsType
indicates a decision observation (check obsType against the decision enum, e.g.
models.ObservationDecision or the project’s decision constant) and otherwise set
Rejected to nil/empty; update the struct literal that creates obs
(ParsedObservation, field Rejected, using obsType and params.Rejected) so the
Rejected field is conditional on obsType == models.ObservationDecision.
---
Nitpick comments:
In `@plugin/openclaw-engram/src/client.ts`:
- Around line 126-131: Create a named interface DecisionSearchObservation for
the inline observation type currently used in the searchDecisions signature,
replace the inline Array<{ title?: string; narrative?: string; concepts?:
string[]; rejected?: string[] }> with DecisionSearchObservation[] in the Promise
return type of searchDecisions, and export the interface if it will be used
elsewhere; update any other references that expect the old inline type to use
DecisionSearchObservation to avoid future desyncs.
In `@plugin/openclaw-engram/src/hooks/after-tool-call.ts`:
- Around line 106-141: The contradiction detection currently uses substring
checks (contentLower.includes) in the loop over rejectedItems and in the
fallback block that builds rejectedWord from rejectionPatterns, causing false
positives; update both places (the rejectedItems handling and the fallback
parsing logic around rejectionPatterns/rejectedWord) to match whole words or
word boundaries (e.g., use a word-boundary regex like \bterm\b) instead of plain
includes, and add a small stop-word filter (common short words such as "not",
"instead", "rather", "avoid", "use", etc.) to skip obvious non-content tokens as
well as keeping the existing minimum length check to avoid reporting on
punctuation/short tokens; ensure the warning still references obs.title or the
narrative excerpt as before when a true word-boundary match is found.
🪄 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: 87620583-1f5a-4030-81df-d25c2b3c225f
📒 Files selected for processing (10)
internal/db/gorm/migrations.gointernal/db/gorm/models.gointernal/db/gorm/observation_store.gointernal/mcp/server.gointernal/mcp/tools_memory.gointernal/search/manager.gopkg/models/observation.goplugin/openclaw-engram/package.jsonplugin/openclaw-engram/src/client.tsplugin/openclaw-engram/src/hooks/after-tool-call.ts
|
@codex review |
…ion detection - Add NOT NULL constraint to observations.rejected JSONB column for stable JSON contract - Extract DecisionSearchObservation and DecisionSearchResponse named interfaces in client.ts - Use word-boundary regex (\bterm\b) in contradiction detection instead of substring includes - Add stop-word filter to contradiction detection to reduce false positives
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/openclaw-engram/src/hooks/after-tool-call.ts`:
- Around line 108-111: Функция containsWholeWord использует границы слов \b,
которые не работают для терминов, оканчивающихся или начинающихся на non-word
символы (например C++, C#, .NET); замените шаблон RegExp в containsWholeWord на
проверку границ через "не-символ слова или строка" вместо \b — например собрать
паттерн, экранировав term (как сейчас) и окружив его выражениями, эквивалентными
(^|[^A-Za-z0-9_]) и ($|[^A-Za-z0-9_]) — чтобы матчить термин на границах строки
или рядом с non-word символами; обновите вызов new RegExp(...) в
containsWholeWord соответственно.
🪄 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: 10ac741b-8843-44c5-bafd-24699d97845a
📒 Files selected for processing (3)
internal/db/gorm/migrations.goplugin/openclaw-engram/src/client.tsplugin/openclaw-engram/src/hooks/after-tool-call.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/openclaw-engram/src/client.ts
- internal/db/gorm/migrations.go
| function containsWholeWord(text: string, term: string): boolean { | ||
| const escaped = term.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| return new RegExp(`\\b${escaped}\\b`).test(text); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript RegExp, does \bterm\b fail for terms that start or end with non-word characters (for example: "C++", "C#", ".NET")?
💡 Result:
Yes. In JavaScript, \b is a word-boundary assertion defined purely in terms of “word characters” (\w ≈ [A-Za-z0-9_]) vs “non-word characters” (\W). \b matches only at a position where the left and right sides differ: one side is \w and the other is \W (or start/end of string as a non-word side). (developer.mozilla.org)
So \bterm\b fails (or behaves unexpectedly) when the term starts or ends with a non-word character:
- Ends with punctuation:
C++,C#
\bC\+\+\b/\bC#\btypically fails at the ending\b, because the character right before it (+or#) is not a word character, so there’s no\w→\Wtransition at that position. (developer.mozilla.org) - Starts with punctuation:
.NET
\b\.NET\btypically fails at the starting\b, because the boundary before.is between two non-word sides (start-of-string counts as non-word, and.is non-word), so it’s not a\b. (developer.mozilla.org) - Gotcha (false positives): a pattern like
/\b(c\+\+|c)\b/can also wrongly matchc#, because\baftercis true before#(sincecis\wand#is\W). (gitlab.com)
If you want “whole token” matching for these, use explicit non-word/start/end guards instead of \b, e.g. (?<!\w)C\+\+(?!\w) or (^|[^\w])C\+\+($|[^\w]) (depending on whether you want to consume separators and whether lookbehind is acceptable). (developer.mozilla.org)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Word_boundary_assertion?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Word_boundary_assertion?utm_source=openai
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Word_boundary_assertion?utm_source=openai
- 4: https://gitlab.com/gitlab-org/gitlab/-/issues/197958?utm_source=openai
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Word_boundary_assertion?utm_source=openai
🏁 Script executed:
cat -n plugin/openclaw-engram/src/hooks/after-tool-call.ts | sed -n '100,160p'Repository: thebtf/engram
Length of output: 2816
containsWholeWord не матчится для терминов с символами на краях слова
На строке 110 шаблон \b${escaped}\b не срабатывает для терминов вроде C++, C#, .NET. В JavaScript \b — это граница слова, определяемая только через word-символы (\w ≈ [A-Za-z0-9_]). \b матчится только там, где одна сторона содержит \w, а другая — \W (или конец строки). Если термин заканчивается на + или # (non-word символы), граница \b не срабатывает, и детектор противоречий пропускает реальные ошибки.
Предлагаемое исправление
function containsWholeWord(text: string, term: string): boolean {
const escaped = term.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
- return new RegExp(`\\b${escaped}\\b`).test(text);
+ return new RegExp(`(?:^|\\W)${escaped}(?:$|\\W)`).test(text);
}📝 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 containsWholeWord(text: string, term: string): boolean { | |
| const escaped = term.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| return new RegExp(`\\b${escaped}\\b`).test(text); | |
| } | |
| function containsWholeWord(text: string, term: string): boolean { | |
| const escaped = term.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| return new RegExp(`(?:^|\\W)${escaped}(?:$|\\W)`).test(text); | |
| } |
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 109-109: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\b${escaped}\\b)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/openclaw-engram/src/hooks/after-tool-call.ts` around lines 108 - 111,
Функция containsWholeWord использует границы слов \b, которые не работают для
терминов, оканчивающихся или начинающихся на non-word символы (например C++, C#,
.NET); замените шаблон RegExp в containsWholeWord на проверку границ через
"не-символ слова или строка" вместо \b — например собрать паттерн, экранировав
term (как сейчас) и окружив его выражениями, эквивалентными (^|[^A-Za-z0-9_]) и
($|[^A-Za-z0-9_]) — чтобы матчить термин на границах строки или рядом с non-word
символами; обновите вызов new RegExp(...) в containsWholeWord соответственно.
Add marked + DOMPurify pipeline via useMarkdown composable; replace plain-text event.body rendering in IssueDetailView with sanitized v-html and scoped Tailwind markdown styles (code, pre, lists, links, paragraphs).
* feat(ui): render markdown in issue body and comments (#13) Add marked + DOMPurify pipeline via useMarkdown composable; replace plain-text event.body rendering in IssueDetailView with sanitized v-html and scoped Tailwind markdown styles (code, pre, lists, links, paragraphs). * fix(ui): address gemini review findings on markdown rendering - Replace deprecated marked.setOptions() with marked.use() (marked v5+) - Use marked.parseSync() for explicit synchronous parsing - Add break-words class to prevent overflow on long URLs/code - Add h1/h2/h3 heading styles to markdown-body scope (Tailwind resets) Reviewer comment ids: PRRC_kwDORY-ek8629gpn, PRRC_kwDORY-ek8629gpr, PRRC_kwDORY-ek8629gpt Thread ids: PRRT_kwDORY-ek856XwQ-, PRRT_kwDORY-ek856XwRA, PRRT_kwDORY-ek856XwRC --------- Co-authored-by: Kirill Turanskiy <thebtf@users.noreply.github.com>
1 participant
Summary
Rejected []stringfield toParsedObservation,Observation, and GORM modelALTER TABLE observations ADD COLUMN rejected JSONB DEFAULT '[]'store_memoryMCP tool accepts optionalrejectedarray parameterdecisionssearch includesrejectedin resultsTest plan
store_memorywithrejected=["redux"]stores structured datadecisionsquery returnsrejectedfield[])rejectedsilently ignoredgo build ./cmd/worker/compiles cleanlySummary by CodeRabbit
Примечания к выпуску
Новые возможности
Bug Fixes / Улучшения
Chores