fix(hooks): expose observation IDs in injected context (#32)#139
Conversation
|
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 (3)
ОбзорВерсия плагина обновлена с 3.7.4 на 3.7.5. В двух хук-файлах добавлены идентификаторы наблюдений в строки заголовков: в разделе "Project Memory" сессии и в секции "Relevant Knowledge From Previous Sessions" при формировании запроса пользователя. Изменения
Оценка трудоёмкости код-ревью🎯 1 (Тривиально) | ⏱️ ~3 минуты Возможно связанные PR
Стихотворение
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 updates the engram plugin to version 3.7.5, adding observation IDs to the context builder, and implements Markdown rendering in the UI using the marked and dompurify libraries. Feedback recommends omitting the ID tag entirely when an ID is missing to prevent agent confusion from placeholder characters. Additionally, it is recommended to use marked.use() instead of the deprecated marked.setOptions() for global configuration in the new useMarkdown composable.
| if (i < fullCount) { | ||
| const narrative = escapeXmlTags(getString(observation.narrative)); | ||
| contextBuilder += `## ${i + 1}. [${typeLabel}] ${title}${scopeTag}\n`; | ||
| contextBuilder += `## ${i + 1}. [${typeLabel}] ${title}${scopeTag} (id:${observation.id || '?'})\n`; |
There was a problem hiding this comment.
Using a placeholder like ? when an ID is missing might confuse the agent or lead to invalid tool calls if it tries to use the placeholder as an ID. It is better to omit the ID tag entirely if the ID is not available.
| contextBuilder += `## ${i + 1}. [${typeLabel}] ${title}${scopeTag} (id:${observation.id || '?'})\n`; | |
| contextBuilder += `## ${i + 1}. [${typeLabel}] ${title}${scopeTag}${observation.id ? ` (id:${observation.id})` : ''}\n`; |
| const scopeTag = (typeof obs.scope === 'string' && obs.scope === 'global') ? ' [GLOBAL]' : ''; | ||
|
|
||
| contextBuilder += `## ${idx}. [${obsType}] ${title}${scopeTag}${scoreTag}\n`; | ||
| contextBuilder += `## ${idx}. [${obsType}] ${title}${scopeTag}${scoreTag} (id:${obs.id || '?'})\n`; |
There was a problem hiding this comment.
Similar to the session-start hook, using ? as a fallback for missing IDs can be problematic for agents. Omit the ID section if the ID is not present to ensure the agent only sees valid identifiers.
| contextBuilder += `## ${idx}. [${obsType}] ${title}${scopeTag}${scoreTag} (id:${obs.id || '?'})\n`; | |
| contextBuilder += `## ${idx}. [${obsType}] ${title}${scopeTag}${scoreTag}${obs.id ? ` (id:${obs.id})` : ''}\n`; |
| marked.setOptions({ | ||
| breaks: true, // GFM line breaks | ||
| gfm: true, // GitHub Flavored Markdown | ||
| }) |
There was a problem hiding this comment.
marked.setOptions is deprecated in marked v18. It is recommended to use marked.use() for global configuration.
marked.use({\n breaks: true, // GFM line breaks\n gfm: true, // GitHub Flavored Markdown\n})![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)
ui/src/views/IssueDetailView.vue (1)
358-358: Не вызывайте markdown-рендерер прямо из шаблона.Line 358 пересчитывает
renderMarkdown(event.body)при каждом реактивном рендере страницы. Это повторно гоняет parse + sanitize по всей timeline даже при локальных UI-изменениях. Лучше подготовить HTML один раз вcomputed/buildTimelineи здесь биндингить уже готовое поле.♻️ Минимальное направление для рефактора
- <div v-if="event.body" class="markdown-body mt-1 text-sm text-gray-600 dark:text-gray-400" v-html="renderMarkdown(event.body)" /> + <div v-if="event.body" class="markdown-body mt-1 text-sm text-gray-600 dark:text-gray-400" v-html="event.htmlBody" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/views/IssueDetailView.vue` at line 358, Шаблон вызывает renderMarkdown(event.body) прямо в разметке, что пересчитывает парсинг/санитизацию при каждом рендере; исправьте это, вычислив HTML один раз и привязывая готовое поле: добавьте computed-свойство или расширьте buildTimeline, чтобы для каждого event заполнить event.renderedBody = renderMarkdown(event.body) (или вернуть структуру с renderedBody), и в шаблоне замените v-html="renderMarkdown(event.body)" на v-html="event.renderedBody" (используйте тот же идентификатор event и функцию renderMarkdown из компонента).ui/src/composables/useMarkdown.ts (1)
5-13: Изолируйте конфигmarkedот глобального singleton-а.Line 5 меняет настройки общего экземпляра
markedна уровне модуля. Сейчас это работает, но любой другой импортmarkedв UI получит эти опции неявно. Уmarkedесть поддержка отдельных экземпляров черезnew Marked(...), поэтому конфиг лучше держать локально в этом composable. (github.com)♻️ Возможный рефактор
-import { marked } from 'marked' +import { Marked } from 'marked' import DOMPurify from 'dompurify' -// Configure marked for safe defaults -marked.setOptions({ +const markdown = new Marked({ breaks: true, // GFM line breaks gfm: true, // GitHub Flavored Markdown }) export function renderMarkdown(text: string): string { if (!text) return '' - const html = marked.parse(text, { async: false }) as string + const html = markdown.parse(text, { async: false }) as string return DOMPurify.sanitize(html) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/composables/useMarkdown.ts` around lines 5 - 13, Не настраивать глобальный singleton marked: вместо вызова marked.setOptions(...) создайте локальный экземпляр через new Marked({ breaks: true, gfm: true }) внутри этого composable и используйте его в функции renderMarkdown (вызов instance.parse(text, { async: false }) ), затем по-прежнему пропустите результат через DOMPurify.sanitize(html). Обновите импорты/использование так, чтобы renderMarkdown ссылался на локальную переменную экземпляра Marked, а не на глобальный marked.ui/package.json (1)
24-24: Уберите лишний@types/dompurify.
dompurifyуже поставляется со встроенными типами TypeScript, а@types/dompurifyна npm опубликован как stub и прямо говорит, что он не нужен. Оставлять оба пакета — лишний риск рассинхрона типов и ненужный шум в зависимостях. (npmjs.com)♻️ Предлагаемое упрощение
"devDependencies": { "@fortawesome/fontawesome-free": "^6.7.2", - "@types/dompurify": "^3.0.5", "@types/node": "^22.10.2",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/package.json` at line 24, Remove the redundant "@types/dompurify" dependency from package.json (the dependency entry "@types/dompurify") since dompurify ships its own types; delete that line, then reinstall dependencies (npm/yarn/pnpm install) to update the lockfile so the package tree no longer includes the stub types.
🤖 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/engram/hooks/session-start.js`:
- Line 227: The template line building contextBuilder inserts observation.id raw
and can cause prompt injection if id is non-numeric; update the logic around
where contextBuilder is appended (the code using contextBuilder and
observation.id) to validate observation.id with a strict numeric check (e.g.,
ensure it parses to an integer or matches /^\d+$/) and only render the numeric
id when validation passes, otherwise substitute a safe placeholder like '?' or
omit the id; keep the rest of the formatting intact so the output remains
predictable and safe.
---
Nitpick comments:
In `@ui/package.json`:
- Line 24: Remove the redundant "@types/dompurify" dependency from package.json
(the dependency entry "@types/dompurify") since dompurify ships its own types;
delete that line, then reinstall dependencies (npm/yarn/pnpm install) to update
the lockfile so the package tree no longer includes the stub types.
In `@ui/src/composables/useMarkdown.ts`:
- Around line 5-13: Не настраивать глобальный singleton marked: вместо вызова
marked.setOptions(...) создайте локальный экземпляр через new Marked({ breaks:
true, gfm: true }) внутри этого composable и используйте его в функции
renderMarkdown (вызов instance.parse(text, { async: false }) ), затем
по-прежнему пропустите результат через DOMPurify.sanitize(html). Обновите
импорты/использование так, чтобы renderMarkdown ссылался на локальную переменную
экземпляра Marked, а не на глобальный marked.
In `@ui/src/views/IssueDetailView.vue`:
- Line 358: Шаблон вызывает renderMarkdown(event.body) прямо в разметке, что
пересчитывает парсинг/санитизацию при каждом рендере; исправьте это, вычислив
HTML один раз и привязывая готовое поле: добавьте computed-свойство или
расширьте buildTimeline, чтобы для каждого event заполнить event.renderedBody =
renderMarkdown(event.body) (или вернуть структуру с renderedBody), и в шаблоне
замените v-html="renderMarkdown(event.body)" на v-html="event.renderedBody"
(используйте тот же идентификатор event и функцию renderMarkdown из компонента).
🪄 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: 300fc7cc-e02c-4d76-817b-3648400519ac
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
plugin/engram/.claude-plugin/plugin.jsonplugin/engram/hooks/session-start.jsplugin/engram/hooks/user-prompt.jsui/package.jsonui/src/composables/useMarkdown.tsui/src/views/IssueDetailView.vue
| if (i < fullCount) { | ||
| const narrative = escapeXmlTags(getString(observation.narrative)); | ||
| contextBuilder += `## ${i + 1}. [${typeLabel}] ${title}${scopeTag}\n`; | ||
| contextBuilder += `## ${i + 1}. [${typeLabel}] ${title}${scopeTag} (id:${observation.id || '?'})\n`; |
There was a problem hiding this comment.
Валидируйте observation.id перед вставкой в инъецируемый контекст.
Сейчас id подставляется как есть. Если сервер вернёт нечисловое значение, можно сломать формат <engram-context> и получить инъекцию в промпт. Используйте строгую числовую проверку перед рендерингом.
Предлагаемый фикс
- contextBuilder += `## ${i + 1}. [${typeLabel}] ${title}${scopeTag} (id:${observation.id || '?'})\n`;
+ const safeObservationId = Number.isInteger(observation.id) && observation.id > 0
+ ? observation.id
+ : '?';
+ contextBuilder += `## ${i + 1}. [${typeLabel}] ${title}${scopeTag} (id:${safeObservationId})\n`;📝 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.
| contextBuilder += `## ${i + 1}. [${typeLabel}] ${title}${scopeTag} (id:${observation.id || '?'})\n`; | |
| const safeObservationId = Number.isInteger(observation.id) && observation.id > 0 | |
| ? observation.id | |
| : '?'; | |
| contextBuilder += `## ${i + 1}. [${typeLabel}] ${title}${scopeTag} (id:${safeObservationId})\n`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/engram/hooks/session-start.js` at line 227, The template line building
contextBuilder inserts observation.id raw and can cause prompt injection if id
is non-numeric; update the logic around where contextBuilder is appended (the
code using contextBuilder and observation.id) to validate observation.id with a
strict numeric check (e.g., ensure it parses to an integer or matches /^\d+$/)
and only render the numeric id when validation passes, otherwise substitute a
safe placeholder like '?' or omit the id; keep the rest of the formatting intact
so the output remains predictable and safe.
Agents couldn't rate observations because the injected <relevant-memory> block showed "## 1. [DECISION] Title" with no ID. The MCP feedback tool requires feedback(action="rate", id=N) but N was never visible. Now renders as "## 1. [DECISION] Title (id:12345)" in both: - session-start.js (initial context injection) - user-prompt.js (per-prompt context injection) Closes #32. Plugin version bumped to 3.7.5.
thebtf
force-pushed
the
fix/issue-32-feedback-observation-ids
branch
from
f660be0 to
ee693d1
Compare
1 participant
Summary
(id:N)to observation headers in both session-start and user-prompt injectionfeedback(action="rate", id=N)— previously impossible because IDs were not visibleRoot cause
2164 feedback records, 0 positive, 0 negative. Agents saw
## 1. [DECISION] Titlebut no ID to reference in feedback calls.Changes
plugin/engram/hooks/session-start.js:227: add(id:${observation.id})to headerplugin/engram/hooks/user-prompt.js:334: add(id:${obs.id})to headerplugin/engram/.claude-plugin/plugin.json: version 3.7.4 → 3.7.5Test plan
<relevant-memory>block shows(id:NNNNN)after each observation titlefeedback(action="rate", id=N, rating="useful")with a visible IDSummary by CodeRabbit
Улучшения